Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB ...................................................................... Patch Set 5: (2 comments) It might be a red herring, but Jenkins pre-commit tests report on failures in ExactlyOnceSemanticsITest.TestWritesWithExactlyOnceSemanticsWithCrashyNodes: http://jenkins.kudu.apache.org/job/kudu-gerrit/25432/ I ran the new bits built with the source with this patch saw it became significantly flaky, while dist-test dashboard hasn't reported anything like that for this test for a long time: http://dist-test.cloudera.org:8080/ The logs for the DEBUG build could be downloaded from here: http://dist-test.cloudera.org/job?job_id=jenkins-slave.1651169101.3480026 Did you mind taking a look at what's going on with that test? Thanks! http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc@973 PS4, Line 973: } > This follow an example in Batcher::CheckForFinishedFlush() on how to access Yup, in Batcher::CheckForFinishedFlush() the lock was necessary to guard access to the state variables. http://gerrit.cloudera.org:8080/#/c/18451/5/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/18451/5/src/kudu/client/batcher.cc@970 PS5, Line 970: sp::shared_ptr<KuduSession> session = weak_session_.lock(); : if (session) { C+17 nit: as a best practice to keep variables local only to the relevant code scope, you could move the 'session' under the if() clause: if (sp::shared_ptr<KuduSession> session = weak_session_.lock()) { session->data_->UpdateWriteOpMetrics(rpc.resp().resource_metrics()); } -- To view, visit http://gerrit.cloudera.org:8080/18451 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3 Gerrit-Change-Number: 18451 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 28 Apr 2022 21:34:12 +0000 Gerrit-HasComments: Yes
