- Notifications
You must be signed in to change notification settings - Fork690
Wip/wan tx grouping module 1#7083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1407b2a to214dc76CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Inlined a few comments. Overall looks like a good direction! Here are some general comments and tips:
- Prefer interfaces over abstract methods.
- Standardize on one library instead of mixing them (such as AssertJ for assertions).
- Is there a better word than "txgrouping" for module name and packages?
- Declare variables and APIs with interfaces rather than implementations.
- Always look for an interface to use instead of an implementation (such as InternalCache instead of GemFireCacheImpl) in both tests and product code.
- Always minimize the scope of anything (class, method, variable, constant).
| importstaticorg.assertj.core.api.Assertions.assertThat; | ||
| importstaticorg.junit.Assert.assertEquals; | ||
| importstaticorg.junit.Assert.assertNotNull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please use AssertJ for all assertions. TheassertEquals can just useassertThat(a).isEqualTo(b) andassertNotNull becomesassertThat(a).isNotNull();
| protectedPropertiescreateLocatorConfig(intsystemId,intlocatorPort,intremoteLocatorPort) { | ||
| Propertiesconfig =newProperties(); | ||
| config.setProperty(MCAST_PORT,"0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the default so you can delete the line withMCAST_PORT
| if (regionSize !=r.keySet().size()) { | ||
| await().untilAsserted(() ->assertThat(r.keySet().size()).isEqualTo(regionSize)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would delete the if-statement and just use the await. The if statement is redundant... the await will also immediately complete successfully if the if-statement is true.
| await() | ||
| .untilAsserted(() ->assertThat(regionQueue.size()).isEqualTo(expectedQueueSize)); | ||
| } | ||
| ArrayList<Integer>stats =newArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remember to declare as the weakest interface you need:
List<Integer> stats = new ArrayList<>();| assertThat(creates).isEqualTo(gatewayReceiverStats.getCreateRequest()); | ||
| } | ||
| protectedvoiddoTxPutsWithRetryIfError(StringregionName,finallongputsPerTransaction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you should try to simplify any new method with this many calculations, for-loops, do-whiles and try-blocks. If you really need all of those blocks then try extracting the nested blocks to their own short little methods.
| try { | ||
| for (AsyncInvocation<?>asyncInvocation :asyncInvocations) { | ||
| asyncInvocation.await(); | ||
| } | ||
| }catch (InterruptedExceptione) { | ||
| fail("Interrupted"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't catch an exception and usefail. Instead just addthrows InterruptedException to the test and delete the try-catch lines.
| }finally { | ||
| exp.remove(); | ||
| exp1.remove(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IgnoredException implementsAutoCloseable so you can use it in try-with-resources syntax. Using import-static andaddIgnoredException(Class) tidies it up nicely:
import static org.apache.geode.test.dunit.IgnoredException.addIgnoredException;try (IgnoredException ie1 = addIgnoredException(RegionDestroyedException.class); IgnoredException ie2 = addIgnoredException(ForceReattemptException.class)) { // ...}| +statistics.getUnprocessedEventsRemovedByPrimary())).isEqualTo(eventsReceived); | ||
| } | ||
| privateBooleankillPrimarySender(StringsenderId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The dunit testing framework originally required primitive wrappers, so I think you may have copied some of this from elsewhere. It's better to just use simple primitives for return types and parameters now, such asboolean instead ofBoolean.
| importorg.apache.geode.internal.cache.wan.GatewaySenderException; | ||
| importorg.apache.geode.internal.cache.wan.MutableGatewaySenderAttributes; | ||
| publicabstractclassCommonTxGroupingGatewaySenderFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you should make this an interface instead of an abstract class. Thestatic validator method would then become adefault implementation which would probably only ever be overridden by a test.
albertogpzDec 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What would be the benefit of using an interface over an abstract class here?
Given that it provides helper static methods I tend to think it is better to define it is a class but I have no strong argument in favor of it.
| while (true) { | ||
| for (Iterator<Map.Entry<TransactionId,Integer>>iter = | ||
| incompleteTransactionIdsInBatch.entrySet().iterator();iter.hasNext();) { | ||
| Map.Entry<TransactionId,Integer>pendingTransaction =iter.next(); | ||
| TransactionIdtransactionId =pendingTransaction.getKey(); | ||
| intbucketId =pendingTransaction.getValue(); | ||
| List<Object>events = | ||
| peekEventsWithTransactionId(partitionedRegion,bucketId,transactionId); | ||
| for (Objectobject :events) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Try to extract any nested loops to their own smaller methods. It helps keep things better organized and more readable.
| } | ||
| @Override | ||
| publicsynchronizedvoidremove()throwsCacheException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At first glance it looks like a lot of the remove method content is reimplemented here. Is there a way break up the original with some extension points to deduce the duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SeeNordix#17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice!
| final@NotNullGatewaySenderAttributesImplattributes) { | ||
| finalintmyDSId =cache.getDistributionManager().getDistributedSystemId(); | ||
| intmyDSId; | ||
| if (cacheinstanceofCacheCreation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
MaybeCacheCreation should be fixed to behave like any otherInternalCache rather than making consumer know subtile differences in behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You mean to implement inCacheCreation thegetDistributionManager() method as follows?
@Override public DistributionManager getDistributionManager() { return InternalDistributedSystem.getAnyInstance().getDistributionManager() }| addIgnoredException("could not get remote locator"); | ||
| StringcreateCommandString = | ||
| "create gateway-sender --id=sender1 --remote-distributed-system-id=1 --parallel --group-transaction-events=true"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't seem modular yet. Is this testing deprecated--group-transaction-events and work is pending to add at--type=TxGroupingParallelGatewaySender?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can find an example of modularizing this in the first POC. Follow the newtype property on thecreate gateway-sender command.
https://github.com/pivotal-jbarrett/geode/blob/wip/wan-spi/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/CreateGatewaySenderCommand.java#L83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done. We'll have to see how we reflect this in the documentation as we will need documentation specific for the module.
@albertogpz Can you please rebase this from develop. There as a fixed added there to address issues with Gradle builds. |
jake-at-work commentedDec 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@albertogpz we need to extract out the TX batching specific elements form the |
Thanks for your comments. |
1850646 toee9b459Comparealbertogpz commentedDec 2, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@pivotal-jbarrett As I said in a previous e-mail, I do not think we can make these changes without breaking backward compatibility. The feature was production ready at 1.14 so we can expect that it is used by customers. Therefore, we cannot just ignore those changes when interworking with 1.14 servers. |
f238c66 tod6d80b2Compare520d955 to1ac52a6Comparethis PR appears to be abandoned, can it be closed? |
1ac52a6 todd7f827Compare
It is not abandoned. Still waiting for comments after review changes. I have rebased the branch to remove the conflicts with develop. |
7a03267 to13ea059Compare13ea059 to4ce6664Compare@albertogpz and@kirklund: I know this has been dormant for a bit, but I believe it still has merit and would certainly be happy to see it included in a future release.@JinwooHwang and I have been reviewing outstanding PRs and are wondering if you have a moment to return to this one and give us your assessment. Is this still something you'd like to see merged? |
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion underASF 2.0?