Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14871 )
Change subject: fs: separate out directory management code
......................................................................
Patch Set 7:
(3 comments)
> Patch Set 6:
>
> (3 comments)
>
> Looks like a lot of code motion. Anything of substance changed in the
> existing methods that were moved from DataDir{Manager} to Dir{Manager}?
No, this patch is only focused on moving code around for better reuse (and
reusing DirManager in DataDirManager in doing so).
http://gerrit.cloudera.org:8080/#/c/14871/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/14871/6/src/kudu/fs/data_dirs.cc@220
PS6, Line 220: : DirManager(env, opts.metric_entity ?
: unique_ptr<DirMetrics>(new
DataDirMetrics(opts.metric_entity)) : nullptr,
: FLAGS_fs_max_thread_count_per_data_dir,
: opts, std::move(canonicalized_data_roots)) {}
> Is it unsafe to pass opts by move because of these read accesses? What are
Here's a test I did:
struct Foo {
string foo_inner;
};
struct Bar {
Bar(string s, Foo f)
: bar_inner(s),
f(std::move(f)) {}
string bar_inner;
Foo foo;
};
TEST(TestMove, TestMove) {
Foo f = { "hello" };
Bar b(f.foo_inner, std::move(f));
LOG(INFO) << Substitute("bar_inner: '$0' foo_inner: '$1'", b.bar_inner,
b.foo.foo_inner);
}
I1213 19:20:20.950659 107070 tablet-test.cc:103] bar_inner: '' foo_inner:
'hello'
This means that bar_inner isn't getting a copy of f's foo_inner, even though
bar_inner is supposedly initialized before Bar::foo. Coming back to the code
snippet at hand, this means opt.metric_entity might be initialized as nullptr
even though the DirManager's DirMetrics is initialized before DirManager::opts.
I vaguely recall seeing that referencing A in the same statement as
std::move(A) is undefined or at least unspecified, though I don't see the C++
rule that says so.
http://gerrit.cloudera.org:8080/#/c/14871/6/src/kudu/fs/data_dirs.cc@327
PS6, Line 327: }
: return Status::OK();
> Maybe invert with the above for clarity (i.e. reduced nesting)?
Done
http://gerrit.cloudera.org:8080/#/c/14871/6/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14871/6/src/kudu/fs/fs_manager.cc@a411
PS6, Line 411:
> Not sure why we aren't setting this anymore?
Oversight on my part -- this isn't strictly necessary since the correct default
is set in the constructor of DataDirManagerOptions, though I'll keep it in for
clarity.
--
To view, visit http://gerrit.cloudera.org:8080/14871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I633e1e32845b08eb24c5327a04af344b579b186a
Gerrit-Change-Number: 14871
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Andrew Wong <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 14 Dec 2019 03:49:51 +0000
Gerrit-HasComments: Yes
