Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h File src/kudu/util/cache.h: http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: MetricsResetBehavior nit (bikeshedding): maybe, name this ExistingMetricsPolicy ? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: enum nit (bikeshedding): any specific reason not to use 'enum class' here? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@76 PS2, Line 76: kKeepFirstMetrics nit (bikeshedding): maybe, name this kKeep (assuming the enum is called ExistingMetricsPolicy)? Or the idea is to have snapshots of various metrics in the future? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@81 PS2, Line 81: kForceResetMetrics nit (bikeshedding): what that 'Force' stands for semantically? Could 'kReset' be a good enough name for this (assuming the enum is called ExistingMetricsPolicy)? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@85 PS2, Line 85: metrics_behavior Should it be kReset by default? http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.cc File src/kudu/util/cache.cc: http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.cc@578 PS2, Line 578: return; Does it make sense to leave CHECK(IsGTest()) under this 'if()' clause, since (1) we expect this method to be called with kKeep only from tests (2) not setting the provided metrics might point to an issue in non-test environments? -- To view, visit http://gerrit.cloudera.org:8080/17636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61 Gerrit-Change-Number: 17636 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Jun 2021 23:44:22 +0000 Gerrit-HasComments: Yes
