Andrew Wong has posted comments on this change. Change subject: separate DataDirManager from BlockManagers ......................................................................
Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 146: bm_.reset(); > Why does the block manager have to be destroyed separately up here? Why isn Done http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 289: LOG(INFO) << "Constructing a new DataDirManager"; > Still need this? Done Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity))); > Ah, I see, this is because DataDirMetrics takes the MetricEntity by cref. A Done Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity))); > warning: passing result of std::move() as a const reference argument; no mo Done PS7, Line 339: canonicalized_data_roots.insert(canonicalized_data_roots.end(), : canonicalized_data_roots_set.begin(), : canonicalized_data_roots_set.end()); > Isn't this logically the same as the returned values from the calls to FsM Not quite, it removes duplicates, but in any case, I've taken this out and moved it back to the FsManager. Line 344: dd_manager->reset(new DataDirManager(env, opts, canonicalized_data_roots)); > warning: parameter 'opts' is passed by value and only copied once; consider Done PS7, Line 350: const int kFlagCreateTestHolePunch = 0x1; : const int kFlagCreateFsync = 0x2; > But why bother with flags at all? Flags are a means of defining an API and Ah, makes sense, way clearer. Done. http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS6, Line 229: // Shuts down all directories' thread pools. : void Shutdown(); : : // Waits on all directories' thread pools. : void WaitOnClosures(); : : // Initializes the data directories on disk. : // : // Returns an error if initialized directories already exist. : Status Create(); : : // Opens existing data roots from disk and indexes the files found. : // > Would it be possible to convert the static Init and the Create and/or Open Done. It was mainly a matter of ordering of canonicalization. I.e. the FsManager needs the canonicalized roots before doing anything; I thought I could push some of it to the directory manager, but I think to do a complete job on that might require making the WAL dir known to the directory manager too. Originally I'd wanted to have the directory manager handle the canonicalization failures, but I realize we might be able to get by doing it at the FsManager level (e.g. by prepending an unsanitized prefix to the non-canonicalized path). In any case, I've taken out the canonicalization changes since I don't think they're necessary for this patch, and probably won't be for handling failures at startup. http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS7, Line 200: Defaultas to fales > Defaults to false Done Line 351: DataDirManager(Env* env, > warning: function 'kudu::fs::DataDirManager::DataDirManager' has a definiti Done Line 378: // The canonicalized roots provided at construction time, taken verbatim. > I understand what this means, but "construction time" is a little weird bec Done. Ah interesting point; how about, "provided to the constructor" -- To view, visit http://gerrit.cloudera.org:8080/7602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
