You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2021/06/25 05:23:45 UTC
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17636
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
[master] allow setting BlockCacheMetrics twice
In some cases, the way we call Cache::SetMetrics() isn't threadsafe.
Typically, we only call this once per cache, but this can be a useful
call to repeat, e.g. if we're constructing a new server that
instantiates its own metric entity but uses the same cache. This usually
doesn't happen since most Caches are tied to a single server. However,
the BlockCache is special in that it's a singleton, so every
instantiation of a server in the same process will use the same cache.
This was the cause of KUDU-2165, which was resolved by only allowing the
first caller to set the metrics. While it prevented a race, this meant
that subsequently started servers would not correctly register metrics.
I have an upcoming patch that will start two servers from the same
process -- effectively restarting a master after running a tablet copy
from the same process as a means to automate the addition of a master to
a cluster. To allow this, this patch adds an option to force the
resetting of the cache metrics.
Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/table_locations_cache.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 97 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/1
--
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: newchange
Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Gerrit-Change-Number: 17636
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 4: Code-Review+1
--
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: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Jul 2021 00:09:15 +0000
Gerrit-HasComments: No
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
[master] allow setting BlockCacheMetrics twice
In some cases, the way we call Cache::SetMetrics() isn't threadsafe.
Typically, we only call this once per cache, but this can be a useful
call to repeat, e.g. if we're constructing a new server that
instantiates its own metric entity but uses the same cache. This usually
doesn't happen since most Caches are tied to a single server. However,
the BlockCache is special in that it's a singleton, so every
instantiation of a server in the same process will use the same cache.
This was the cause of KUDU-2165, which was resolved by only allowing the
first caller to set the metrics. While it prevented a race, this meant
that subsequently started servers would not correctly register metrics.
I have an upcoming patch that will start two servers from the same
process -- effectively restarting a master after running a tablet copy
from the same process as a means to automate the addition of a master to
a cluster. To allow this, this patch adds an option to force the
resetting of the cache metrics.
Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Reviewed-on: http://gerrit.cloudera.org:8080/17636
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/table_locations_cache.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 101 insertions(+), 18 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Gerrit-Change-Number: 17636
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17636
to look at the new patch set (#2).
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
[master] allow setting BlockCacheMetrics twice
In some cases, the way we call Cache::SetMetrics() isn't threadsafe.
Typically, we only call this once per cache, but this can be a useful
call to repeat, e.g. if we're constructing a new server that
instantiates its own metric entity but uses the same cache. This usually
doesn't happen since most Caches are tied to a single server. However,
the BlockCache is special in that it's a singleton, so every
instantiation of a server in the same process will use the same cache.
This was the cause of KUDU-2165, which was resolved by only allowing the
first caller to set the metrics. While it prevented a race, this meant
that subsequently started servers would not correctly register metrics.
I have an upcoming patch that will start two servers from the same
process -- effectively restarting a master after running a tablet copy
from the same process as a means to automate the addition of a master to
a cluster. To allow this, this patch adds an option to force the
resetting of the cache metrics.
Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/table_locations_cache.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 98 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/2
--
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: newpatchset
Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Gerrit-Change-Number: 17636
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc
File src/kudu/master/master_options.cc:
http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc@59
PS1, Line 59: }
> Nit: This could be part of the constructor initliazer list
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: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 23:10:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17636
to look at the new patch set (#5).
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
[master] allow setting BlockCacheMetrics twice
In some cases, the way we call Cache::SetMetrics() isn't threadsafe.
Typically, we only call this once per cache, but this can be a useful
call to repeat, e.g. if we're constructing a new server that
instantiates its own metric entity but uses the same cache. This usually
doesn't happen since most Caches are tied to a single server. However,
the BlockCache is special in that it's a singleton, so every
instantiation of a server in the same process will use the same cache.
This was the cause of KUDU-2165, which was resolved by only allowing the
first caller to set the metrics. While it prevented a race, this meant
that subsequently started servers would not correctly register metrics.
I have an upcoming patch that will start two servers from the same
process -- effectively restarting a master after running a tablet copy
from the same process as a means to automate the addition of a master to
a cluster. To allow this, this patch adds an option to force the
resetting of the cache metrics.
Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/table_locations_cache.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 101 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/5
--
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: newpatchset
Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Gerrit-Change-Number: 17636
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 23:44:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 26 Jun 2021 06:39:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17636/3/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/17636/3/src/kudu/master/master-test.cc@220
PS3, Line 220: correctl
> typo
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: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 06 Jul 2021 22:14:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17636
to look at the new patch set (#3).
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
[master] allow setting BlockCacheMetrics twice
In some cases, the way we call Cache::SetMetrics() isn't threadsafe.
Typically, we only call this once per cache, but this can be a useful
call to repeat, e.g. if we're constructing a new server that
instantiates its own metric entity but uses the same cache. This usually
doesn't happen since most Caches are tied to a single server. However,
the BlockCache is special in that it's a singleton, so every
instantiation of a server in the same process will use the same cache.
This was the cause of KUDU-2165, which was resolved by only allowing the
first caller to set the metrics. While it prevented a race, this meant
that subsequently started servers would not correctly register metrics.
I have an upcoming patch that will start two servers from the same
process -- effectively restarting a master after running a tablet copy
from the same process as a means to automate the addition of a master to
a cluster. To allow this, this patch adds an option to force the
resetting of the cache metrics.
Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/table_locations_cache.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 98 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/3
--
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: newpatchset
Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Gerrit-Change-Number: 17636
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17636/3/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/17636/3/src/kudu/master/master-test.cc@220
PS3, Line 220: correcly
typo
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 06 Jul 2021 21:41:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17636
to look at the new patch set (#4).
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
[master] allow setting BlockCacheMetrics twice
In some cases, the way we call Cache::SetMetrics() isn't threadsafe.
Typically, we only call this once per cache, but this can be a useful
call to repeat, e.g. if we're constructing a new server that
instantiates its own metric entity but uses the same cache. This usually
doesn't happen since most Caches are tied to a single server. However,
the BlockCache is special in that it's a singleton, so every
instantiation of a server in the same process will use the same cache.
This was the cause of KUDU-2165, which was resolved by only allowing the
first caller to set the metrics. While it prevented a race, this meant
that subsequently started servers would not correctly register metrics.
I have an upcoming patch that will start two servers from the same
process -- effectively restarting a master after running a tablet copy
from the same process as a means to automate the addition of a master to
a cluster. To allow this, this patch adds an option to force the
resetting of the cache metrics.
Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/table_locations_cache.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 98 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/17636/4
--
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: newpatchset
Gerrit-Change-Id: I4bb9c6f382a26654f2d324676506441f370ffe61
Gerrit-Change-Number: 17636
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc
File src/kudu/master/master_options.cc:
http://gerrit.cloudera.org:8080/#/c/17636/1/src/kudu/master/master_options.cc@59
PS1, Line 59: block_cache_metrics_reset_behavior_ = Cache::kKeepFirstMetrics;
Nit: This could be part of the constructor initliazer list
--
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: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 20:29:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 2:
(2 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@85
PS2, Line 85: metrics_behavior
> I kept it as kKeep since that's the current behavior in all cases. As for d
Well, in nvm_cache.cc the current behavior is more kReset than kKeep. Talking about kKeep in cache.cc is only for the case of running SetMetrics() in the context of a test, otherwise a process would crash with SIGABRT. With that, I guess we are free to converge with whatever behavior makes more sense. I agree: it seems kKeep would be a good option for tests.
Yeah, introducing parameters by default is a tricky business, so I'm OK with not introducing a default by any other means.
http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/nvm_cache.cc@662
PS2, Line 662: return;
What do you think of adding the CHECK(IsGTest()) here as it's in cache.cc?
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Jul 2021 00:04:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/nvm_cache.cc@662
PS2, Line 662: (metri
> Done
Thank you!
--
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: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Jul 2021 01:20:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] allow setting BlockCacheMetrics twice
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17636 )
Change subject: [master] allow setting BlockCacheMetrics twice
......................................................................
Patch Set 5:
(2 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@85
PS2, Line 85: metrics_policy)
> Well, in nvm_cache.cc the current behavior is more kReset than kKeep. Talki
Ack
http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/17636/2/src/kudu/util/nvm_cache.cc@662
PS2, Line 662: (metri
> What do you think of adding the CHECK(IsGTest()) here as it's in cache.cc?
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: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Jul 2021 00:28:44 +0000
Gerrit-HasComments: Yes