Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice ...................................................................... Patch Set 3: (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: enum > nit (bikeshedding): any specific reason not to use 'enum class' here? Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@69 PS2, Line 69: class ExistingMetric > nit (bikeshedding): maybe, name this ExistingMetricsPolicy ? Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@76 PS2, Line 76: kKeep, > nit (bikeshedding): maybe, name this kKeep (assuming the enum is called Exi Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@81 PS2, Line 81: kReset, > nit (bikeshedding): what that 'Force' stands for semantically? Could 'kRes Done http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/cache.h@85 PS2, Line 85: metrics_policy) > Should it be kReset by default? I kept it as kKeep since that's the current behavior in all cases. As for default arguments in pure virtual methods, I've been advised against it before, so I'll keep it as is: https://stackoverflow.com/questions/12139786/good-practice-default-arguments-for-pure-virtual-method 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: // probably better than spurious failures. > Does it make sense to leave CHECK(IsGTest()) under this 'if()' clause, sinc Done -- 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: 3 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: Sat, 26 Jun 2021 06:39:42 +0000 Gerrit-HasComments: Yes
