ISPN-7367 CommandAckCollector Improvements #4774

pruivo · 2017-01-18T13:48:27Z

https://issues.jboss.org/browse/ISPN-7367

pruivo · 2017-01-18T13:48:52Z

PerfAck: library + dist + writes + no-tx

pruivo · 2017-01-19T16:31:25Z

run performance tests please
library dist writes

ghost · 2017-01-19T17:32:16Z

Performance tests run successfully. Link to the results here.

galderz · 2017-01-23T06:36:26Z

@pruivo Results are in but they seem worse for 100 threads?

pruivo · 2017-01-23T10:11:57Z

run performance tests please
library dist writes

ghost · 2017-01-23T10:37:43Z

Performance tests didn't finish successfully. @Holmistr, can you review it?

jmarkos · 2017-01-23T12:04:39Z

run performance tests please
library dist writes

ghost · 2017-01-23T14:39:45Z

Performance tests run successfully. Link to the results here.

pruivo · 2017-01-23T14:45:54Z

personal note: results above used commit f5b4b6b

galderz · 2017-01-23T14:54:58Z

Whoa!! Huge increase in perf!

galderz · 2017-01-23T14:55:15Z

TX writes 100 threads still lower, more work to come?

pruivo · 2017-01-23T14:58:38Z

@galderz . short answer, yes.
I'm not trusting 100% with the results. it is too much based on the changes. Also, the snapshot didn't pick today's commit and I want to compare with no-bundler.
Transactional caches is not the goal of the PR and they shouldn't be affected.

galderz · 2017-01-23T15:08:46Z

@pruivo So, do you want to tidy up and include in Beta2? Or wait?

pruivo · 2017-01-23T15:09:52Z

@galderz you can release Beta2 without it :) I want to make sure the results are real or not, and I need Dan to review it as well.

galderz · 2017-01-23T15:10:25Z

Fair point

galderz · 2017-01-23T15:10:45Z

Removed milestone

pruivo · 2017-01-25T09:39:40Z

run performance tests please
library dist writes

ghost · 2017-01-25T10:27:40Z

Performance tests run successfully. Link to the results here.

pruivo · 2017-01-25T11:41:03Z

run performance tests please
library dist writes

ghost · 2017-01-25T12:31:20Z

Performance tests run successfully. Link to the results here.

pruivo · 2017-02-03T10:34:31Z

@danberindei updated!

pruivo · 2017-02-03T16:29:44Z

@danberindei updated again :)

pruivo · 2017-02-03T16:30:00Z

run performance tests please
library dist writes

danberindei

Some minor cosmetic issues.

danberindei · 2017-02-03T16:46:21Z

.../src/main/java/org/infinispan/interceptors/distribution/TriangleDistributionInterceptor.java

@@ -354,7 +362,7 @@ private Object primaryOwnerWrite(InvocationContext context, DataWriteCommand com
   }

   private void logCommandSequence(CommandInvocationId id, int segment, long sequence) {
-         log.tracef("Command %s got sequence %s for segment %s", id, sequence, segment);
+      log.tracef("Command %s got sequence %s for segment %s", id, sequence, segment);


I'd move the if (trace) check in here.

danberindei · 2017-02-03T16:46:51Z

.../src/main/java/org/infinispan/interceptors/distribution/TriangleDistributionInterceptor.java

-                     command.getTopologyId());
+               .createMultiKeyCollector(command.getCommandInvocationId().getId(), filter.primaries.keySet(),
+                     filter.backups,
+                     command.getTopologyId(), timeoutNanos);


Join these lines?

danberindei · 2017-02-03T17:05:01Z

core/src/main/java/org/infinispan/util/concurrent/CommandAckCollector.java

+
+      private void traceBackupAck(int topologyId, Address from) {
+         log.tracef("[Collector#%s] Backup ACK. Address=%s, TopologyId=%s (expected=%s)",
+               id, from, topologyId, this.topologyId);


Here too, if (trace) could be in the method.

danberindei · 2017-02-03T17:06:49Z

core/src/main/java/org/infinispan/util/concurrent/CommandAckCollector.java

   @Inject
   public void inject(
-         @ComponentName(value = KnownComponentNames.TIMEOUT_SCHEDULE_EXECUTOR) ScheduledExecutorService timeoutExecutor,
-         Configuration configuration) {
+         @ComponentName(value = KnownComponentNames.TIMEOUT_SCHEDULE_EXECUTOR) ScheduledExecutorService timeoutExecutor) {


Don't need value =

danberindei · 2017-02-03T17:10:33Z

core/src/main/java/org/infinispan/util/logging/Log.java

@@ -1473,7 +1473,7 @@ TimeoutException timeoutWaitingForView(int expectedViewId, int currentViewId,
   //removed unused message (id=426)

   @Message(value = "Timeout after %s waiting for acks. Id=%s", id = 427)
-   TimeoutException timeoutWaitingForAcks(String timeout, CommandInvocationId id);
+   TimeoutException timeoutWaitingForAcks(String timeout, Long id);


The id could be a primitive long.

ghost · 2017-02-03T18:04:25Z

Performance tests run successfully. Link to the results here.

pruivo · 2017-02-03T18:08:21Z

@danberindei updated!

danberindei · 2017-02-04T15:40:56Z

CI build hanged in InitialClusterSizeTest, but I don't think it's related:
https://issues.jboss.org/browse/ISPN-7439

danberindei · 2017-02-13T15:36:58Z

The AffinityTest failure seems specific to this branch:
http://ci.infinispan.org/project.html?projectId=Infinispan&testNameId=8733223218815095024&tab=testDetails

pruivo · 2017-02-14T12:34:31Z

finally understood the issue. The CommandAckCollector cannot be a global component, it must be a per-cache component since it needs the cache topology updates to keeps its members updated.

pruivo · 2017-02-14T16:04:03Z

run performance tests please
library dist writes

ghost · 2017-02-14T20:20:19Z

Performance tests run successfully. Link to the results here.

danberindei · 2017-02-22T14:22:44Z

.../src/main/java/org/infinispan/interceptors/distribution/TriangleDistributionInterceptor.java

@@ -91,12 +91,11 @@
   private static final Log log = LogFactory.getLog(TriangleDistributionInterceptor.class);
   private static final boolean trace = log.isTraceEnabled();
   private CommandAckCollector commandAckCollector;
+   private final InvocationExceptionFunction onPutMapNoLocalEntriesException = this::onPutMapNoLocalEntriesException;


I think it's better to group the lambdas at the end then mixing them with the injected components.

no idea why the reformat put the field there... but it does it consistently

pruivo · 2017-02-22T15:08:36Z

updated

danberindei · 2017-02-22T15:24:13Z

Integrated, thanks Pedro!

pruivo added Performance ACK required! Ready for Review labels Jan 18, 2017

pruivo added this to the 9.0.0.Beta2 milestone Jan 18, 2017

pruivo force-pushed the t_7367 branch from 111ef13 to c924b1a Compare January 19, 2017 10:49

pruivo mentioned this pull request Jan 20, 2017

[PERF] testing with triangle #4781

Closed

pruivo force-pushed the t_7367 branch from c924b1a to bed73e6 Compare January 23, 2017 10:11

galderz removed this from the 9.0.0.Beta2 milestone Jan 23, 2017

pruivo force-pushed the t_7367 branch from cd6fb91 to 57bfaa8 Compare January 25, 2017 09:38

pruivo force-pushed the t_7367 branch from 57bfaa8 to 8fe0b3d Compare January 25, 2017 11:40

pruivo force-pushed the t_7367 branch from 8fe0b3d to 25b81fe Compare January 26, 2017 10:10

danberindei approved these changes Feb 3, 2017

View reviewed changes

pruivo force-pushed the t_7367 branch from 8190e83 to 100512f Compare February 3, 2017 16:29

pruivo added Ready for Review and removed Changes Required labels Feb 3, 2017

danberindei suggested changes Feb 3, 2017

View reviewed changes

pruivo force-pushed the t_7367 branch from 100512f to 6ca7cc9 Compare February 3, 2017 18:08

danberindei approved these changes Feb 4, 2017

View reviewed changes

pruivo added On ice and removed Ready for Review labels Feb 13, 2017

pruivo force-pushed the t_7367 branch from 6ca7cc9 to 62546b2 Compare February 14, 2017 14:16

pruivo added Performance ACK required! Ready for Review and removed On ice labels Feb 14, 2017

pruivo removed the Performance ACK required! label Feb 17, 2017

danberindei reviewed Feb 22, 2017

View reviewed changes

ISPN-7367 CommandAckCollector Improvements

ea0293b

pruivo force-pushed the t_7367 branch from 62546b2 to ea0293b Compare February 22, 2017 15:07

danberindei closed this Feb 22, 2017

ISPN-7367 CommandAckCollector Improvements #4774

ISPN-7367 CommandAckCollector Improvements #4774

Conversation

pruivo commented Jan 18, 2017

pruivo commented Jan 18, 2017

pruivo commented Jan 19, 2017

ghost commented Jan 19, 2017

galderz commented Jan 23, 2017

pruivo commented Jan 23, 2017

ghost commented Jan 23, 2017

jmarkos commented Jan 23, 2017

ghost commented Jan 23, 2017

pruivo commented Jan 23, 2017

galderz commented Jan 23, 2017

galderz commented Jan 23, 2017

pruivo commented Jan 23, 2017

galderz commented Jan 23, 2017

pruivo commented Jan 23, 2017

galderz commented Jan 23, 2017

galderz commented Jan 23, 2017

pruivo commented Jan 25, 2017

ghost commented Jan 25, 2017

pruivo commented Jan 25, 2017

ghost commented Jan 25, 2017

pruivo commented Feb 3, 2017

pruivo commented Feb 3, 2017

pruivo commented Feb 3, 2017

danberindei left a comment

Choose a reason for hiding this comment

danberindei Feb 3, 2017

Choose a reason for hiding this comment

danberindei Feb 3, 2017

Choose a reason for hiding this comment

danberindei Feb 3, 2017

Choose a reason for hiding this comment

danberindei Feb 3, 2017

Choose a reason for hiding this comment

danberindei Feb 3, 2017

Choose a reason for hiding this comment

ghost commented Feb 3, 2017

pruivo commented Feb 3, 2017

danberindei commented Feb 4, 2017

danberindei commented Feb 13, 2017

pruivo commented Feb 14, 2017

pruivo commented Feb 14, 2017

ghost commented Feb 14, 2017

danberindei Feb 22, 2017

Choose a reason for hiding this comment

pruivo Feb 22, 2017

Choose a reason for hiding this comment

pruivo commented Feb 22, 2017

danberindei commented Feb 22, 2017