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 16: (1 comment) http://gerrit.cloudera.org:8080/#/c/13426/15/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/15/src/kudu/tserver/ts_tablet_manager.cc@1308 PS15, Line 1308: lock_stats_.lock_shared(); : ReportedTabletStatsPB stats_pb = replica->stats_pb(); : lock_stats_.unlock_shared(); > The 'lock_stats_' is both used by writing(update stats by heartbeat thread) I agree that the usage of lock_stats_ is fine for UpdateTabletStatsIfNecessary. However, I think it's unnecessary here, though, if we push locking into the TabletReplica. The purpose of the locking over here is to prevent concurrent calls to stats_pb(). That would be unnecessary if stats_pb() itself were threadsafe through TabletReplica-internal locking. The benefit of locking inside TabletReplica is that in my opinion, it makes it easier to reason about the usage of lock_stats_ if it only protected concurrent calls to UpdateTabletStatsIfNecessary. In general, it is easier to think about locking when the lock's purpose is specific, instead of using a single lock for many different purposes. In this case, we're using it to protect UpdateTabletStatsIfNecessary _and_ to protect stats_pb(). As such, I would prefer limiting the usage of lock_stats_ to just UpdateTabletStatsIfNecessary() and guarding stats_pb() with a simple_spinlock or something inside TabletReplica. -- 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: 16 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 04:11:25 +0000 Gerrit-HasComments: Yes
