Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 17: (3 comments) Thanks for making the change! http://gerrit.cloudera.org:8080/#/c/13426/17/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/13426/17/src/kudu/tablet/tablet_replica.cc@837 PS17, Line 837: if (stats_pb_.on_disk_size() != pb.on_disk_size() || We should make sure that _all_ access to stats_pb_ is guarded by lock_. Maybe lock right before this conditional block? http://gerrit.cloudera.org:8080/#/c/13426/17/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/17/src/kudu/tserver/ts_tablet_manager.h@398 PS17, Line 398: // Used to update the latest tablet stats. nit: How about "Ensures that we only update stats from a single thread at a time." http://gerrit.cloudera.org:8080/#/c/13426/17/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/17/src/kudu/tserver/ts_tablet_manager.cc@1307 PS17, Line 1307: { : ReportedTabletStatsPB stats_pb = replica->stats_pb(); : if (stats_pb.IsInitialized()) { : *reported_tablet->mutable_stats() = std::move(stats_pb); : } : } nit: don't need to introduce the nested scope here now that we're not locking. -- To view, visit http://gerrit.cloudera.org:8080/13426 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74406ab7cca7c22fda455c328b8ee9989a6b2d99 Gerrit-Change-Number: 13426 Gerrit-PatchSet: 17 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Tue, 25 Jun 2019 15:37:11 +0000 Gerrit-HasComments: Yes
