You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2021/05/04 22:56:12 UTC
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17400
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
[util] Add a function to fetch non-default flags as a map
We already have a function that returns non-default flags
as a vector of strings. This change adds a function
that returns non-default flags as a map refactoring
and reusing the existing logic.
This function is used in add master tool to bring
up the new master as it's unnecessary to pass all
the flags to the kudu master. This is similar to
master run implementation in master_runner.cc.
Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
---
M src/kudu/tools/tool_action_master.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
3 files changed, 40 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/17400/1
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@566
PS2, Line 566: vector<CommandLineFlagInfo>
nit: Might be worth making 'vector<CommandLineFlagInfo>' a typedef.
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@596
PS2, Line 596: // leading to unexpected value in "flag.name".
Not sure I understand this change. The order of the flags could be different due to GetAllFlags? Still don't quite understand how that leads to an unexpected value in 'flag.name'.
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 May 2021 01:27:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@594
PS2, Line 594: // Instead of "flags_by_name.emplace(flag.name, std::move(flag))" using
: // following approach as order of evaluation of parameters could be different
: // leading to unexpected value in "flag.name".
> How about using EmplaceOrUpdate() from gutil/map-util.h? Same below.
Thanks for that post. However it's not related to the problem I'm trying to fix. See the explanation above.
IIUC EmplaceOrUpdate()/insert_or_assign() is susceptible to the same issue of order of evaluation of function parameters because of use of std::move().
The way I've implemented is one way, other way I could think about is explicitly using a copy of the flag.name and then using any of the new EmplaceOrUpdate()/insert_or_assign().
const auto flag_name = flag.name;
EmplaceOrUpdate(&flags_by_name, flag_name, std::move(flag));
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@596
PS2, Line 596: // leading to unexpected value in "flag.name".
> flags_by_name.emplace(flag.name, std::move(flag))
Above approach was used previously L590. In the line above, it's not guaranteed that flag.name will be evaluated before std::move(flag) as it's dependent on the compiler. If std::move(flag) is evaluated first then the value in flag.name could be empty/garbage. That's what I meant by "order of evaluation of parameters".
I remember this being the case in pre-C++11 world and from the first paragraph still looks to be the case (I haven't dived into the details but the solution is simple and consequences are dire so why risk it).
https://en.cppreference.com/w/cpp/language/eval_order
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 May 2021 17:16:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17400
to look at the new patch set (#2).
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
[util] Add a function to fetch non-default flags as a map
We already have a function that returns non-default flags
as a vector of strings. This change adds a function
that returns non-default flags as a map refactoring
and reusing the existing logic.
This function is used in add master tool to bring
up the new master as it's unnecessary to pass all
the flags to the kudu master. This is similar to
master run implementation in master_runner.cc.
Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
---
M src/kudu/tools/tool_action_master.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
3 files changed, 40 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/17400/2
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@594
PS2, Line 594: // Instead of "flags_by_name.emplace(flag.name, std::move(flag))" using
: // following approach as order of evaluation of parameters could be different
: // leading to unexpected value in "flag.name".
How about using EmplaceOrUpdate() from gutil/map-util.h? Same below.
Under the hood it's insert_or_assign https://www.fluentcpp.com/2018/12/11/overview-of-std-map-insertion-emplacement-methods-in-cpp17/
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 May 2021 01:48:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@566
PS2, Line 566: vector<CommandLineFlagInfo>
> nit: Might be worth making 'vector<CommandLineFlagInfo>' a typedef.
I don't see a need.
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@581
PS2, Line 581: args.str().empty()
> nit: this is not part of this particular patch, but this operation is needl
My bad I missed this comment. Will resolve in next change.
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@594
PS2, Line 594: // Instead of "flags_by_name.emplace(flag.name, std::move(flag))" using
: // following approach as order of evaluation of parameters could be different
: // leading to unexpected value in "flag.name".
> Ah, indeed I misinterpreted the comment -- makes sense that we're trying to
Done
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@596
PS2, Line 596: // leading to unexpected value in "flag.name".
> Ah ok, thanks for the clear explanation. Agreed that this approach is safer
Done
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 May 2021 16:07:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
[util] Add a function to fetch non-default flags as a map
We already have a function that returns non-default flags
as a vector of strings. This change adds a function
that returns non-default flags as a map refactoring
and reusing the existing logic.
This function is used in add master tool to bring
up the new master as it's unnecessary to pass all
the flags to the kudu master. This is similar to
master run implementation in master_runner.cc.
Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Reviewed-on: http://gerrit.cloudera.org:8080/17400
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Mahesh Reddy <mr...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tools/tool_action_master.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
3 files changed, 40 insertions(+), 21 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, but someone else must approve
Mahesh Reddy: Looks good to me, but someone else must approve
Grant Henke: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@594
PS2, Line 594: // Instead of "flags_by_name.emplace(flag.name, std::move(flag))" using
: // following approach as order of evaluation of parameters could be different
: // leading to unexpected value in "flag.name".
> Thanks for that post. However it's not related to the problem I'm trying to
Ah, indeed I misinterpreted the comment -- makes sense that we're trying to avoid using the variable in the same line that it's moved. That could be undefined.
+1 for the explicit approach. Hopefully it's clear from the code even without a comment that we're trying to avoid using 'flag' in the same statement as its move() call.
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 May 2021 18:37:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/1/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/1/src/kudu/util/flags.cc@571
PS1, Line 571: been
Fix this grammatical error.
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 May 2021 23:03:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@581
PS2, Line 581: args.str().empty()
> My bad I missed this comment. Will resolve in next change.
Not a problem -- this wasn't a part of this patch, so feel free to address it another patch or just ignore :)
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 May 2021 16:24:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 May 2021 02:53:33 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/1/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/1/src/kudu/util/flags.cc@571
PS1, Line 571: has
> Fix this grammatical error.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 May 2021 23:17:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@596
PS2, Line 596: // leading to unexpected value in "flag.name".
> > flags_by_name.emplace(flag.name, std::move(flag))
Ah ok, thanks for the clear explanation. Agreed that this approach is safer.
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 May 2021 20:57:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add a function to fetch non-default flags as a map
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17400 )
Change subject: [util] Add a function to fetch non-default flags as a map
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/17400/2/src/kudu/util/flags.cc@581
PS2, Line 581: args.str().empty()
nit: this is not part of this particular patch, but this operation is needlessly expensive. Why don't we simply add "\n" unconditionally after outputting a flag's info after line 586? Another option is to use a simple boolean to check whether that's the very first flag being output.
--
To view, visit http://gerrit.cloudera.org:8080/17400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7c9fc4de9be05110dba5ca02f48d46b6b474b7
Gerrit-Change-Number: 17400
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 May 2021 04:37:55 +0000
Gerrit-HasComments: Yes