Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wip/wan tx grouping module 1 #7083

Draft
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

albertogpz
Copy link
Contributor

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 build run 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 under ASF 2.0?

Copy link
Contributor

@kirklund kirklund left a 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:

  1. Prefer interfaces over abstract methods.
  2. Standardize on one library instead of mixing them (such as AssertJ for assertions).
  3. Is there a better word than "txgrouping" for module name and packages?
  4. Declare variables and APIs with interfaces rather than implementations.
  5. Always look for an interface to use instead of an implementation (such as InternalCache instead of GemFireCacheImpl) in both tests and product code.
  6. Always minimize the scope of anything (class, method, variable, constant).

Comment on lines 26 to 28
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
Copy link
Contributor

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. The assertEquals can just use assertThat(a).isEqualTo(b) and assertNotNull becomes assertThat(a).isNotNull();


protected Properties createLocatorConfig(int systemId, int locatorPort, int remoteLocatorPort) {
Properties config = new Properties();
config.setProperty(MCAST_PORT, "0");
Copy link
Contributor

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 with MCAST_PORT

Comment on lines 292 to 294
if (regionSize != r.keySet().size()) {
await().untilAsserted(() -> assertThat(r.keySet().size()).isEqualTo(regionSize));
}
Copy link
Contributor

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 = new ArrayList<>();
Copy link
Contributor

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());
}

protected void doTxPutsWithRetryIfError(String regionName, final long putsPerTransaction,
Copy link
Contributor

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.

Comment on lines 191 to 160
try {
for (AsyncInvocation<?> asyncInvocation : asyncInvocations) {
asyncInvocation.await();
}
} catch (InterruptedException e) {
fail("Interrupted");
}
Copy link
Contributor

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 use fail. Instead just add throws InterruptedException to the test and delete the try-catch lines.

Comment on lines 467 to 415
} finally {
exp.remove();
exp1.remove();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IgnoredException implements AutoCloseable so you can use it in try-with-resources syntax. Using import-static and addIgnoredException(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);
}

private Boolean killPrimarySender(String senderId) {
Copy link
Contributor

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 as boolean instead of Boolean.

import org.apache.geode.internal.cache.wan.GatewaySenderException;
import org.apache.geode.internal.cache.wan.MutableGatewaySenderAttributes;

public abstract class CommonTxGroupingGatewaySenderFactory {
Copy link
Contributor

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. The static validator method would then become a default implementation which would probably only ever be overridden by a test.

Copy link
Contributor Author

@albertogpz albertogpz Dec 1, 2021

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.

Comment on lines 64 to 72
while (true) {
for (Iterator<Map.Entry<TransactionId, Integer>> iter =
incompleteTransactionIdsInBatch.entrySet().iterator(); iter.hasNext();) {
Map.Entry<TransactionId, Integer> pendingTransaction = iter.next();
TransactionId transactionId = pendingTransaction.getKey();
int bucketId = pendingTransaction.getValue();
List<Object> events =
peekEventsWithTransactionId(partitionedRegion, bucketId, transactionId);
for (Object object : events) {
Copy link
Contributor

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
public synchronized void remove() throws CacheException {
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Nordix#17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -256,7 +257,13 @@ public GatewaySenderFactory setEnforceThreadsConnectSameReceiver(

static void validate(final @NotNull InternalCache cache,
final @NotNull GatewaySenderAttributesImpl attributes) {
final int myDSId = cache.getDistributionManager().getDistributedSystemId();
int myDSId;
if (cache instanceof CacheCreation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe CacheCreation should be fixed to behave like any other InternalCache rather than making consumer know subtile differences in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean to implement in CacheCreation the getDistributionManager() method as follows?

  @Override
  public DistributionManager getDistributionManager() {
    return InternalDistributedSystem.getAnyInstance().getDistributionManager()
  }

addIgnoredException("could not get remote locator");

String createCommandString =
"create gateway-sender --id=sender1 --remote-distributed-system-id=1 --parallel --group-transaction-events=true";
Copy link
Contributor

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?

Copy link
Contributor

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 new type property on the create 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

Copy link
Contributor Author

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.

@jake-at-work
Copy link
Contributor

@albertogpz Can you please rebase this from develop. There as a fixed added there to address issues with Gradle builds.

@jake-at-work
Copy link
Contributor

jake-at-work commented Dec 1, 2021

@albertogpz we need to extract out the TX batching specific elements form the GatewaySenderEventImpl object. I replaced it with a implementation defined metadata object. See TxBatchMetadata from my previous example on making this a module. This gets set on the event object when constructed via an override on AbstractGatewaySenderEventProcessor. See example here.

@albertogpz
Copy link
Contributor Author

Inlined a few comments. Overall looks like a good direction! Here are some general comments and tips:

1. Prefer interfaces over abstract methods.

2. Standardize on one library instead of mixing them (such as AssertJ for assertions).

3. Is there a better word than "txgrouping" for module name and packages?

4. Declare variables and APIs with interfaces rather than implementations.

5. Always look for an interface to use instead of an implementation (such as InternalCache instead of GemFireCacheImpl) in both tests and product code.

6. Always minimize the scope of anything (class, method, variable, constant).

Thanks for your comments.
Regarding the name, I do not have one that I really like. Other alternatives could be: txeventsgrouping, grouptxevents, txaware,...
If you like any of these or have any other proposal, please, let me know.

@albertogpz albertogpz force-pushed the wip/wan-tx-grouping-module_1 branch 3 times, most recently from 1850646 to ee9b459 Compare December 2, 2021 15:16
@albertogpz
Copy link
Contributor Author

albertogpz commented Dec 2, 2021

@albertogpz we need to extract out the TX batching specific elements form the GatewaySenderEventImpl object. I replaced it with a implementation defined metadata object. See TxBatchMetadata from my previous example on making this a module. This gets set on the event object when constructed via an override on AbstractGatewaySenderEventProcessor. See example here.

@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.
Am I missing something?

@albertogpz albertogpz force-pushed the wip/wan-tx-grouping-module_1 branch 5 times, most recently from 520d955 to 1ac52a6 Compare December 15, 2021 14:48
@onichols-pivotal
Copy link
Contributor

this PR appears to be abandoned, can it be closed?

@albertogpz
Copy link
Contributor Author

this PR appears to be abandoned, can it be closed?

It is not abandoned. Still waiting for comments after review changes. I have rebased the branch to remove the conflicts with develop.

@albertogpz albertogpz force-pushed the wip/wan-tx-grouping-module_1 branch 3 times, most recently from 7a03267 to 13ea059 Compare February 8, 2022 09:44
@kirklund kirklund removed their request for review April 4, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants