MINOR: Update Trogdor ConnectionStressWorker status at the end of execution by stanislavkozlovski · Pull Request #6445 · apache/kafka

stanislavkozlovski · 2019-03-14T16:21:49Z

I was testing this worker locally and found that it would not report correct numbers after it shutting down due to the durationMs passing.

Status before patch:

{"tasks":{"connect":{"state":"DONE","spec":{"class":"org.apache.kafka.trogdor.workload.ConnectionStressSpec","startMs":1552576318156,"durationMs":60000,"clientNode":["node0"],"bootstrapServers":"localhost:9092","targetConnectionsPerSec":100,"numThreads":10,"action":"CONNECT"},"startedMs":1552576318159,"doneMs":1552576378366,"cancelled":false,"status":{"totalConnections":4021,"totalFailedConnections":0,"connectsPerSec":100.19186205865498}}

"connect2":{"state":"RUNNING","spec":{"class":"org.apache.kafka.trogdor.workload.ConnectionStressSpec","startMs":1552576480870,"durationMs":60000,"clientNode":["node0"],"bootstrapServers":"localhost:9092","targetConnectionsPerSec":200,"numThreads":10,"action":"CONNECT"},"startedMs":1552576480870,"status":{"totalConnections":8021,"totalFailedConnections":0,"connectsPerSec":200.3797246995928}}}}

Status after patch:

"tasks":{"connect_newer1":{"state":"DONE","spec":{"class":"org.apache.kafka.trogdor.workload.ConnectionStressSpec","startMs":1552579728411,"durationMs":60000,"clientNode":["node0"],"bootstrapServers":"localhost:9092","targetConnectionsPerSec":100,"numThreads":10,"action":"CONNECT"},"startedMs":1552579728412,"doneMs":1552579788645,"cancelled":false,"status":{"totalConnections":6010,"totalFailedConnections":0,"connectsPerSec":100.19004434368019}}

"connect_newer2":{"state":"DONE","spec":{"class":"org.apache.kafka.trogdor.workload.ConnectionStressSpec","startMs":1552579472885,"durationMs":60000,"clientNode":["node0"],"bootstrapServers":"localhost:9092","targetConnectionsPerSec":200,"numThreads":10,"action":"CONNECT"},"startedMs":1552579472891,"doneMs":1552579533114,"cancelled":false,"status":{"totalConnections":11980,"totalFailedConnections":0,"connectsPerSec":200.31434973079624}}}}

…cution

stanislavkozlovski · 2019-03-14T16:23:52Z

        doneFuture.complete("");
        workerExecutor.shutdownNow();
        workerExecutor.awaitTermination(1, TimeUnit.DAYS);
+        synchronized (ConnectionStressWorker.this) {


Even though calling updateStatus() here is thread-safe, I think that using the lock here acts as a memory barrier and allows us to access the latest values of the variables. I very well might be wrong though and this may be superfluous

It's not thread-safe without the synchronized, since we're accessing variables that need synchronization. But let's move the synchronized keyword into ConnectionStressWorker#updateStatus, as described above, since that's a bit cleaner

cmccabe · 2019-03-14T16:30:22Z

+     * Update the worker's status.
+     * This method should be called inside a lock on the ConnectionStressWorker object
+     */
+    private void updateStatus(long lastTimeMs) {


This function should be synchronized since it accesses totalConnections, totalFailedConnections, etc. which are only ever accessed under the lock.

cmccabe · 2019-03-14T16:33:16Z

Thanks, @stanislavkozlovski . Great find. I left a few comments.

…dBugs IS2_INCONSISTENT_SYNC warning

stanislavkozlovski · 2019-03-14T16:51:37Z

-        this.totalConnections = 0;
-        this.totalFailedConnections  = 0;
-        this.startTimeMs = TIME.milliseconds();
+        synchronized (ConnectionStressWorker.this) {


Had to add this here otherwise we got a FindBugs warning

IS | Inconsistent synchronization of org.apache.kafka.trogdor.workload.ConnectionStressWorker.nextReportTime; locked 66% of time -- | -- | Bug type IS2_INCONSISTENT_SYNC (click for details) In class org.apache.kafka.trogdor.workload.ConnectionStressWorkerField org.apache.kafka.trogdor.workload.ConnectionStressWorker.nextReportTimeSynchronized 66% of the timeUnsynchronized access at ConnectionStressWorker.java:[line 107]Synchronized access at ConnectionStressWorker.java:[line 124]Synchronized access at ConnectionStressWorker.java:[line 162] IS | Inconsistent synchronization of org.apache.kafka.trogdor.workload.ConnectionStressWorker.startTimeMs; locked 50% of time | Bug type IS2_INCONSISTENT_SYNC (click for details) In class org.apache.kafka.trogdor.workload.ConnectionStressWorkerField org.apache.kafka.trogdor.workload.ConnectionStressWorker.startTimeMsSynchronized 50% of the timeUnsynchronized access at ConnectionStressWorker.java:[line 104]Synchronized access at ConnectionStressWorker.java:[line 120] IS | Inconsistent synchronization of org.apache.kafka.trogdor.workload.ConnectionStressWorker.totalConnections; locked 66% of time | Bug type IS2_INCONSISTENT_SYNC (click for details) In class org.apache.kafka.trogdor.workload.ConnectionStressWorkerField org.apache.kafka.trogdor.workload.ConnectionStressWorker.totalConnectionsSynchronized 66% of the timeUnsynchronized access at ConnectionStressWorker.java:[line 102]Synchronized access at ConnectionStressWorker.java:[line 120]Synchronized access at ConnectionStressWorker.java:[line 120] IS | Inconsistent synchronization of org.apache.kafka.trogdor.workload.ConnectionStressWorker.totalFailedConnections; locked 50% of time | Bug type IS2_INCONSISTENT_SYNC (click for details) In class org.apache.kafka.trogdor.workload.ConnectionStressWorkerField org.apache.kafka.trogdor.workload.ConnectionStressWorker.totalFailedConnectionsSynchronized 50% of the timeUnsynchronized access at ConnectionStressWorker.java:[line 103]Synchronized access at ConnectionStressWorker.java:[line 120]

(see IS2_INCONSISTENT_SYNC in http://findbugs.sourceforge.net/bugDescriptions.html)

cmccabe · 2019-03-15T16:52:20Z

LGTM

cmccabe · 2019-03-15T16:52:38Z

Failed test is kafka.api.UserQuotaTest.testThrottledProducerConsumer, which is not related to this change.

* apache/trunk: MINOR: Retain public constructors of classes from public API (apache#6455) KAFKA-8118; Ensure ZK clients are closed in tests, fix verification (apache#6456) KAFKA-7813: JmxTool throws NPE when --object-name is omitted KAFKA-8114: Wait for SCRAM credential propagation in DelegationTokenEndToEndAuthorizationTest (apache#6452) KAFKA-8111; Set min and max versions for Metadata requests (apache#6451) KAFKA-7855: Kafka Streams Maven Archetype quickstart fails to compile out of the box (apache#6194) MINOR: Update code to not use deprecated methods (apache#6434) MINOR: Update Trogdor ConnectionStressWorker status at the end of execution (apache#6445) KAFKA-8091; Use commitSync to check connection failure in listener update test (apache#6450) KAFKA-7027: Add an overload build method in scala (apache#6373) MINOR: Fix typos in LogValidator (apache#6449) KAFKA-7502: Cleanup KTable materialization logic in a single place (apache#6174) KAFKA-7730; Limit number of active connections per listener in brokers (KIP-402) KAFKA-8091; Remove unsafe produce from dynamic listener update test (apache#6443) MINOR: Fix JavaDocs warnings (apache#6435) MINOR: Better messaging for invalid fetch response (apache#6427) MINOR: Use Java 8 lambdas in KStreamImplTest (apache#6430)

…cution (apache#6445) Reviewers: Colin P. McCabe <cmccabe@apache.org>

MINOR: Update Trogdor ConnectionStressWorker status at the end of exe…

5dd2f91

…cution

stanislavkozlovski commented Mar 14, 2019

View reviewed changes

cmccabe reviewed Mar 14, 2019

View reviewed changes

Make updateStatus synchronized

1166afb

stanislavkozlovski added 2 commits March 14, 2019 18:47

Add synchronization in variable initializations in order to avoid Fin…

ca81397

…dBugs IS2_INCONSISTENT_SYNC warning

Update javadoc

82259ec

stanislavkozlovski commented Mar 14, 2019

View reviewed changes

cmccabe merged commit f20f3c1 into apache:trunk Mar 15, 2019

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019

MINOR: Update Trogdor ConnectionStressWorker status at the end of exe…

274c024

…cution (apache#6445) Reviewers: Colin P. McCabe <cmccabe@apache.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Update Trogdor ConnectionStressWorker status at the end of execution#6445

MINOR: Update Trogdor ConnectionStressWorker status at the end of execution#6445
cmccabe merged 4 commits intoapache:trunkfrom
stanislavkozlovski:minor-improve-trogdor-connection-stress-worker-status

stanislavkozlovski commented Mar 14, 2019

Uh oh!

stanislavkozlovski Mar 14, 2019

Uh oh!

cmccabe Mar 14, 2019

Uh oh!

cmccabe Mar 14, 2019

Uh oh!

cmccabe commented Mar 14, 2019

Uh oh!

stanislavkozlovski Mar 14, 2019 •

edited

Loading

Uh oh!

cmccabe commented Mar 15, 2019

Uh oh!

cmccabe commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

stanislavkozlovski commented Mar 14, 2019

Uh oh!

stanislavkozlovski Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

cmccabe Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

cmccabe Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Mar 14, 2019

Uh oh!

stanislavkozlovski Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Mar 15, 2019

Uh oh!

cmccabe commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stanislavkozlovski Mar 14, 2019 •

edited

Loading