subject:"\[kudu\-CR\] KUDU\-1489\: allow configuration of metadata dir"

[kudu-CR] KUDU-1489: allow configuration of metadata dir

2018-01-18 Thread Todd Lipcon (Code Review)

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

2018-01-18 Thread Todd Lipcon (Code Review)

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

2018-01-17 Thread Adar Dembo (Code Review)

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

2018-01-17 Thread Andrew Wong (Code Review)

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

2018-01-17 Thread Andrew Wong (Code Review)

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

2018-01-17 Thread Adar Dembo (Code Review)

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, , {}, {});
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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Todd Lipcon (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Adar Dembo (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

[kudu-CR] KUDU-1489: allow configuration of metadata dir

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread David Ribeiro Alves (Code Review)

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

2018-01-16 Thread Adar Dembo (Code Review)

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, 
_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 case

[kudu-CR] KUDU-1489: allow configuration of metadata dir

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-16 Thread Andrew Wong (Code Review)

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

2018-01-13 Thread Andrew Wong (Code Review)

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

2018-01-13 Thread Andrew Wong (Code Review)

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

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

[kudu-CR] KUDU-1489: allow configuration of metadata dir

24 matches

Site Navigation

Mail list logo

Footer information