[kudu-CR] KUDU-1489: allow configuration of metadata dir
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Reviewed-on: http://gerrit.cloudera.org:8080/9027 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo Reviewed-by: Todd Lipcon --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 347 insertions(+), 87 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 13 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 18 Jan 2018 20:08:06 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 12: Code-Review+2 Since this is a pretty significant change, maybe get another reviewer too? -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 18 Jan 2018 03:36:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc@265 PS11, Line 265: // We should be able to verify that the metadata is in the WAL root. > Nit: is 'explicate' the right choice here? I substitute it with 'analyze' o You're right. Wanted to say that we should be able to _explicitly see_. I'll go with "verify". http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h@268 PS11, Line 268: // Initializes, sanitizes, and canonicalizes the filesystem roots. > Isn't this exactly the same as FRIEND_TEST(MiniTabletServerTest...) above? Done http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2214 PS11, Line 2214: ASSERT_OK(env_->CreateDir(kTestDir)); > ASSERT_OK Done http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2229 PS11, Line 2229: Status s = RunTool(Substitute("fs dump uuid --fs_wal_dir=$0", opts.wal_root), > ASSERT on the expected status? Ah, yeah. Thought it was void like RunAction*. http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc@764 PS11, Line 764: .AddOptionalParameter("fs_wal_dir") > Since you're adding these, mind reordering the optional parameters to be al Done http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc File src/kudu/tserver/mini_tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc@60 PS11, Line 60: ot), or > Nit: older Done -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 18 Jan 2018 02:24:28 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#12). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 347 insertions(+), 87 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/12 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager-test.cc@265 PS11, Line 265: // We should be able to explicate that the metadata is in the WAL root. Nit: is 'explicate' the right choice here? I substitute it with 'analyze' or 'explain' and the sentence doesn't make sense. http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/fs/fs_manager.h@268 PS11, Line 268: friend class tserver::MiniTabletServerTest_TestFsLayoutEndToEnd_Test; Isn't this exactly the same as FRIEND_TEST(MiniTabletServerTest...) above? Actually, maybe the above has no effect because you should be doing FRIEND_TEST(tserver::MiniTabletServerTest, ...)? http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2214 PS11, Line 2214: env_->CreateDir(kTestDir); ASSERT_OK http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/kudu-tool-test.cc@2229 PS11, Line 2229: RunTool(Substitute("fs dump uuid --fs_wal_dir=$0", opts.wal_root), nullptr, &stderr, {}, {}); ASSERT on the expected status? http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tools/tool_action_fs.cc@764 PS11, Line 764: .AddOptionalParameter("fs_metadata_dir") Since you're adding these, mind reordering the optional parameters to be alphabetically sorted? No backwards compatibility concerns; it'll just show them differently in the help. Same for the other files. http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc File src/kudu/tserver/mini_tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/11/src/kudu/tserver/mini_tablet_server-test.cc@60 PS11, Line 60: younger Nit: older -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 22:24:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#11). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 328 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/11 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc@728 PS10, Line 728: CHECK_NE(first_healthy, -1); // Guaranteed by LoadInstances(). > maybe just make this a CHECK since it'll avoid some ugly stack overwriting Done http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@94 PS10, Line 94: compatibility > typo Done http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@96 PS10, Line 96: exist > nit: exists Done http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@98 PS10, Line 98: TAG_FLAG(fs_metadata_dir, stable); > maybe this should be stable out of the gate? Done -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 07:29:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc@728 PS10, Line 728: DCHECK_NE(first_healthy, -1); // Guaranteed by LoadInstances(). maybe just make this a CHECK since it'll avoid some ugly stack overwriting on the next line, and this isn't any sort of perf-critical code http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@94 PS10, Line 94: compatability typo http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@96 PS10, Line 96: exist nit: exists http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@98 PS10, Line 98: TAG_FLAG(fs_metadata_dir, advanced); maybe this should be stable out of the gate? -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:07:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#10). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 327 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/10 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#9). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 327 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/9 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 7: (9 comments) Done, and added a more integration-y test per David's feedback. http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176 PS6, Line 176: > typo Done http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248 PS6, Line 248: "wal" > could we be more specific? maybe look for full suffix? Done http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255 PS6, Line 255: LOG(INFO) << s.ToString(); > leftover from testing? if you want to print the status on failure you can d Done http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258 PS6, Line 258: : TEST_F(FsManagerTestBase, TestMetadataDirInData > check the status Per Adar's comment, I moved these each into their own test. http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263 PS6, Line 263: : // Creating a brand new FS layout configured with metadata in the first data : // directory emulates the default behavior in Kudu 1.6 and below. : opts.metadata_root = GetTestPath("data"); : ReinitFsManagerWithOpts(opts); > in the case before this one you test with a metadata root path that does no Done http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269 PS6, Line 269: ASSERT_OK(fs_manager()->Open()); : ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(), "data"); > wait, empty string is different from "unset"? why? Mm, not sure I understand, what do you mean by "unset" here? This behavior is for backwards compatibility: older deployments can continue running with no change to their configurations (i.e. with an empty fs_metadata_dir flag) while maintaining that their metadata will be in the first data dir. New deployments will use the fs_wal_dir for metadata if fs_metadata_dir is empty. http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276 PS6, Line 276: ASSERT_OK(fs_manager()->Open()); : ASSERT_STR_CONTAINS(fs_manager()->GetTabletMe > same Ack http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc@534 PS7, Line 534: // Adding a new data dir elsewhere in the list is OK. > This isn't what the test does anymore though; it's just shuffling the order Ah you're right, my bad. Removing it. I'll add a similar test up where we test metadata in the first data root. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256 PS4, Line 256: const string meta_dir_in_data_root = JoinPathSegments(canonicalized_data_fs_roots_[0].path, > My concern is with the following sequence: I see. Step 3 would fail at CreateFileSystemRoots(). The canonicalized roots wouldn't be empty, so this would lead to an AlreadyPresent error. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 17 Jan 2018 02:57:03 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#8). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following tests are added: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to switch back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/mini_tablet_server-test.cc 10 files changed, 316 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/8 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc@534 PS7, Line 534: // Adding a new data dir elsewhere in the list is OK. This isn't what the test does anymore though; it's just shuffling the order of the directories in data_roots. Is that worth testing? If so, reword the comment further. If not, remove this part of the test. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256 PS4, Line 256: const string meta_dir_in_data_root = JoinPathSegments(canonicalized_data_fs_roots_[0].path, > Is your concern that if the FS layout already partially exists, but not com My concern is with the following sequence: 1. I'm going to create an FS with wal_dir=/a, metadata_dir="", data_dirs=/b. 2. First I do "mkdir -p /b/tablet-meta". 3. Then I do FSManager.CreateInitialFileSystemLayout. 4. Then I do FSManager.Open. I think I'll wind up with metadata_dir=/b/tablet-meta instead of /a/tablet-meta, but maybe this is too contrived due to step #2. What would be interesting is if step #2 could be substituted with an otherwise valid Kudu operation. Anyway, my question was "will step #3 fail because of the directory created in step #2"? If so, this is a non-issue. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 21:23:46 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20 PS4, Line 20: It is up to the user to take caution in changing this flag; updating the : flag without also manually moving any existing metadata will cause Kudu : to fail at startup. > Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing fs_metad Good point. No point in bringing it up, I'll take this out. http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29 PS4, Line 29: cases > Nit: case Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176 PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by DataDirManager::LoadInstnaces(). > Can we not depend on this, and instead return a bad status if there are no Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258 PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); > Maybe use different tests so you needn't clean up in between? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271 PS4, Line 271: opts.metadata_root = ""; > Maybe opts.metadata_root.clear() would be more idiomatic? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284 PS4, Line 284: ASSERT_OK(fs_manager()->Open()); > After this could you check that the WAL root and the metadata root aren't t Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288 PS4, Line 288: opts.metadata_root = ""; > .clear() here too. Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517 PS4, Line 517: // Try to open with a new data dir at the front of the list. > Nit: the other "Try to ..." comments here are all followed up by "this shou Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524 PS4, Line 524: // But adding a new data dir elsewhere in the list is OK. > And reword this too. Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90 PS4, Line 90: or the first configured data root if it already contains : // existing metadata. > Would it be clearer if we additionally specified that this fallback behavio Hm, I didn't explicitly call it out, but I reworded it to hopefully clarify that the first data dir is only used if metadata exists there from a previous deployment. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93 PS4, Line 93: DEFINE_string(fs_metadata_dir, "", > There should be a mention here of the fallback behavior for existing system Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94 PS4, Line 94: Must be equivalent to fs_wal_dir or a > I thought this wasn't true anymore? Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95 PS4, Line 95: fs_dataA_dirs > fs_data_dirs Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253 PS4, Line 253: // Check the first data root for metadata. > Could you LOG here what we actually used for the metadata directory, a la L Done http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256 PS4, Line 256: // If there is no metadata in the first data root, use the WAL root. > Hmm, but we wouldn't want to do this fallback when creating a brand new fil Is your concern that if the FS layout already partially exists, but not completely (such that fs_manager()->Open() fails, but fs_manager()->Create() succeeds), we may end up going through CreateInitialFileSystemLayout() with the first data root storing metadata? That could happen if they fail to start up a cluster with Kudu 1.6, upgrade to Kudu 1.7, and then start up successfully. I think I'm ok with this though, although a simple update would be to also check whether the metadata directory is empty, in which case we could use the WAL dir. What do you think? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265 PS4, Line 265: const auto& canonicalized_metadata_root = canonicalized_metadata_fs_root_; > Why do we need this local? Done http://gerrit.cloudera.org:8080/#/
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#7). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. New test cases are added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc 7 files changed, 173 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/7 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 6: (7 comments) first pass. mostly looked at the test. it'd be nice to have a more integration-y test that tests at least the most worrysome cases (downgrade/upgrade most likely scenarios) http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176 PS6, Line 176: LoadInstnaces typo http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248 PS6, Line 248: "wal" could we be more specific? maybe look for full suffix? http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255 PS6, Line 255: LOG(INFO) << s.ToString(); leftover from testing? if you want to print the status on failure you can do so on the assert below http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258 PS6, Line 258: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); check the status http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263 PS6, Line 263: opts.metadata_root = GetTestPath("data"); : ReinitFsManagerWithOpts(opts); : ASSERT_OK(fs_manager()->CreateInitialFileSystemLayout()); : ASSERT_OK(fs_manager()->Open()); : ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(), "data"); in the case before this one you test with a metadata root path that does not exist, before you delete the data/wal could you test that if the metadata root is set to the old value (like in this case) it fails too? http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269 PS6, Line 269: // Opening the FsManager with an empty fs_metadata_dir flag should account : // for the old default and use the first data directory for metadata. wait, empty string is different from "unset"? why? http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276 PS6, Line 276: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); same -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 20:07:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20 PS4, Line 20: It is up to the user to take caution in changing this flag; updating the : flag without also manually moving any existing metadata will cause Kudu : to fail at startup. Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing fs_metadata_dir somehow more dangerous? I guess what I'm asking is: why call this out explicitly if it works the same way as those other flags? http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29 PS4, Line 29: cases Nit: case http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176 PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by DataDirManager::LoadInstnaces(). Can we not depend on this, and instead return a bad status if there are no healthy instances? Seems like a weird dependency since this is a "util" function that ostensibly could be called from anywhere. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258 PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal")); : env_->DeleteRecursively(GetTestPath("data")); Maybe use different tests so you needn't clean up in between? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271 PS4, Line 271: opts.metadata_root = ""; Maybe opts.metadata_root.clear() would be more idiomatic? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284 PS4, Line 284: ASSERT_OK(fs_manager()->Open()); After this could you check that the WAL root and the metadata root aren't the same? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288 PS4, Line 288: opts.metadata_root = ""; .clear() here too. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517 PS4, Line 517: // Try to open with a new data dir at the front of the list. Nit: the other "Try to ..." comments here are all followed up by "this should fail", but this no longer fails. Reword? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524 PS4, Line 524: // But adding a new data dir elsewhere in the list is OK. And reword this too. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90 PS4, Line 90: or the first configured data root if it already contains : // existing metadata. Would it be clearer if we additionally specified that this fallback behavior only takes effect when opening an existing filesystem? That is, when creating a new filesystem, it's just "verbatim, or WAL dir if empty"? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93 PS4, Line 93: DEFINE_string(fs_metadata_dir, "", There should be a mention here of the fallback behavior for existing systems. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94 PS4, Line 94: Must be equivalent to fs_wal_dir or a I thought this wasn't true anymore? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95 PS4, Line 95: fs_dataA_dirs fs_data_dirs http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253 PS4, Line 253: // Check the first data root for metadata. Could you LOG here what we actually used for the metadata directory, a la L243-244? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256 PS4, Line 256: // If there is no metadata in the first data root, use the WAL root. Hmm, but we wouldn't want to do this fallback when creating a brand new filesystem. Does it actually matter? Is it possible for CreateFileSystemLayout to succeed if meta_dir_in_data_root already exists? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265 PS4, Line 265: const auto& canonicalized_metadata_root = canonicalized_metadata_fs_root_; Why do we need this local? http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@421 PS4, Line 421: if (dd_manager_->FindUuidIndexByRoot(canonicalized_metadata_fs_root_.path, &metadata_idx) && I'm confused; why would the DataDirManager be aware of the metadata root, since it's no longer guaranteed to be colocated with the data directory? Oh, this is just for the
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 5: Sorry, pushed accidentally in conjunction with another patch. Reverting to patch set 4. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 18:51:48 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#6). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. It is up to the user to take caution in changing this flag; updating the flag without also manually moving any existing metadata will cause Kudu to fail at startup. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. A new test cases is added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc 7 files changed, 160 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/6 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 4: I think we should merge KUDU-2202 before merging this (will need to update a couple of things), but feel free to review this in it's current form. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 18:48:34 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#4). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. It is up to the user to take caution in changing this flag; updating the flag without also manually moving any existing metadata will cause Kudu to fail at startup. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. A new test cases is added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc 7 files changed, 160 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/4 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#3). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. It is up to the user to take caution in changing this flag; updating the flag without also manually moving any existing metadata will cause Kudu to fail at startup. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. A new test cases is added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h 6 files changed, 143 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/3 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9027 Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice. This first drive, if not performant, can act as a bottleneck; often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. It is up to the user to take caution in changing this flag; updating the flag without also manually moving any existing metadata will cause Kudu to fail at startup. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. A new test cases is added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath with lead to D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h 6 files changed, 142 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/1 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong