[kudu-CR] fs: store opts at construction time

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..

fs: store opts at construction time

This is easier than adding a new member for each new option.

Note: I removed the options destructors so that the compiler would
generate move constructors; see commit 312492d for more information.

Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Reviewed-on: http://gerrit.cloudera.org:8080/8437
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/tablet_copy_client-test.cc
26 files changed, 171 insertions(+), 185 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] fs: store opts at construction time

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@333
PS1, Line 333:   canonicalized_data_fs_roots_, std::move(dm_opts), 
_manager_));
> right, but clang-tidy isn't smart enough to analyze the loop and know that
Makes sense, I just wasn't sure from your comment whether you thought it was a 
false positive, or whether it was a real access-after-move.



--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 18:25:57 +
Gerrit-HasComments: Yes

[kudu-CR] fs: store opts at construction time

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@333
PS1, Line 333:   canonicalized_data_fs_roots_, std::move(dm_opts), 
_manager_));
> But it's not actually accessing dm_opts after the move, is it? The LOG_TIMI
right, but clang-tidy isn't smart enough to analyze the loop and know that it 
always only runs for one iteration. I'll see if I can come up with a 
non-looping variant for LOG_TIMING perhaps



--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 04:32:58 +
Gerrit-HasComments: Yes

[kudu-CR] fs: store opts at construction time

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 02:12:58 +
Gerrit-HasComments: No

[kudu-CR] fs: store opts at construction time

[Adar Dembo (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8437

to look at the new patch set (#2).

Change subject: fs: store opts at construction time
..

fs: store opts at construction time

This is easier than adding a new member for each new option.

Note: I removed the options destructors so that the compiler would
generate move constructors; see commit 312492d for more information.

Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
---
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/open-readonly-fs-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/tablet_copy_client-test.cc
26 files changed, 171 insertions(+), 185 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/8437/2
--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] fs: store opts at construction time

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.h@68
PS1, Line 68:   // 'data_paths', which are both initialized to 'root'.
> nit: note that this is only used in tests?
Done


http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@134
PS1, Line 134: wal_path
> nit: can we use "wal_root" and "data_roots" instead?
Done


http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@320
PS1, Line 320:   if (!opts_.read_only) {
> nit: !read_only()?
Eh, there are quite a few references to opts_.read_only in fs_manager.cc, and I 
think that's OK; read_only() is largely for external callers.



--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 01:44:52 +
Gerrit-HasComments: Yes

[kudu-CR] fs: store opts at construction time

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@333
PS1, Line 333:   canonicalized_data_fs_roots_, std::move(dm_opts), 
_manager_));
> hrm, this seems like it's due to the implementation of LOG_TIMING using a f
But it's not actually accessing dm_opts after the move, is it? The LOG_TIMING 
loop is exited on its second iteration, right?



--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 01:27:51 +
Gerrit-HasComments: Yes

[kudu-CR] fs: store opts at construction time

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 1:

(3 comments)

Some nits

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.h@68
PS1, Line 68:   // 'data_paths', which are both initialized to 'root'.
nit: note that this is only used in tests?


http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@134
PS1, Line 134: wal_path
nit: can we use "wal_root" and "data_roots" instead?
"path" seems particularly overloaded since it can generally refer to any 
arbitrary file/directory. "wal_path" and "data_path" could then potentially be 
misconstrued as a path pointing at a WAL file or some data file, whatever that 
might mean.


http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@320
PS1, Line 320:   if (!opts_.read_only) {
nit: !read_only()?



--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 01:25:05 +
Gerrit-HasComments: Yes

[kudu-CR] fs: store opts at construction time

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8437 )

Change subject: fs: store opts at construction time
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8437/1/src/kudu/fs/fs_manager.cc@333
PS1, Line 333:   canonicalized_data_fs_roots_, std::move(dm_opts), 
_manager_));
> warning: 'dm_opts' used after it was moved [misc-use-after-move]
hrm, this seems like it's due to the implementation of LOG_TIMING using a for 
loop. Maybe we can use some other trick for the macro



--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 01 Nov 2017 01:23:14 +
Gerrit-HasComments: Yes

[kudu-CR] fs: store opts at construction time

[Adar Dembo (Code Review)]

Hello Andrew Wong,

I'd like you to do a code review. Please visit

http://gerrit.cloudera.org:8080/8437

to review the following change.


Change subject: fs: store opts at construction time
..

fs: store opts at construction time

This is easier than adding a new member for each new option.

Note: I removed the options destructors so that the compiler would
generate move constructors; see commit 312492d for more information.

Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
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/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/server/server_base.cc
M src/kudu/tools/tool_action_fs.cc
17 files changed, 136 insertions(+), 152 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/8437/1
--
To view, visit http://gerrit.cloudera.org:8080/8437
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If64ec78f28b5c6879805a682e209d96846f03f27
Gerrit-Change-Number: 8437
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong