You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/07/23 07:55:26 UTC
[kudu-CR] version util: allow period as delimiter for extra
Hello Will Berkeley, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/13897
to review the following change.
Change subject: version_util: allow period as delimiter for extra
......................................................................
version_util: allow period as delimiter for extra
This will permit versions like <major>.<minor>.<maintenance>.<vendor_ver>,
a common pattern used by at least one major Kudu vendor. This shouldn't have
any tangible effect on Kudu as we've always ignored the value of 'extra'.
I tried to reuse the string parsing but ended up with ugly looking code, so
I rewrote it as a DFA (i.e. poor man's regex).
Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
---
M src/kudu/util/version_util-test.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
3 files changed, 83 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/13897/1
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13897
to look at the new patch set (#5).
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
version_util: allow period as delimiter in extra and tighten version syntax
This will permit versions like <major>.<minor>.<maintenance>.<vendor_ver>,
a common pattern used by at least one Kudu vendor. The change shouldn't have
any tangible effect upstream as we've always ignored the value of 'extra'.
The string splitting wasn't cutting it for me so I rewrote the parsing to
use POSIX extended regexes. The major side effect is that we're no longer
loosey goosey with components (i.e. "1. 2 . 3" or "+1.+2.+3"); each must
be a subsequence of nothing but digits. I don't think anyone was relying on
this excessive permissiveness and I find the new code to be more readable.
Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
---
M src/kudu/util/version_util-test.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
3 files changed, 46 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/13897/5
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 3:
> (1 comment)
>
> > It doesn't need to be done in this change, but at some point it
> might be a good idea to decouple version as in artifact version.txt
> type version and feature flag x things is supported version.
> >
> > Currently we use the version in `VersionSupportsRF1Movement`, If
> instead we checked in a version epoch of sorts we could decouple
> these two concepts and avoid weird issues.
>
> I think the issue here was that, at the time that VersionSupportsRF1Movement
> was added, we wanted it to be retroactively true for several
> releases of Kudu. A feature flag can't address that: it'll only
> give you "supports X"=true from the release that added the flag
> rather than a release or two before then.
>
> Alexey would know more about it.
Adar is right: VersionSupportsRF1Movement() was introduced because it was not possible to use feature flags to deal with a bug discovered later on when a few versions have been already released.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:30:34 +0000
Gerrit-HasComments: No
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@65
PS3, Line 65: *
> nit: Should be a +? Or is "1.10.0-" considered valid?
It's valid. The delimiter is chopped off and the 'extra' component is empty. From the unit test:
{ { "1.1.1-", 1, 1, 1, "" }, "1.1.1" },
(that's input, major, minor, maintenance, extra, and reassembled).
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 23:51:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@97
PS3, Line 97: matches[4].rm_eo - (matches[4].rm_s
> Hmm, no. I'm surprised UBSAN didn't complain about undefined behavior.
Seems like substr() should take care of that for us:
If the requested substring extends past the end of the string, or if count == npos, the returned substring is [pos, size()).
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:18:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:16:14 +0000
Gerrit-HasComments: No
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13897/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13897/3//COMMIT_MSG@16
PS3, Line 16: I don't think anyone was relying on
: this excessive permissiveness and I find the new code to be more readable.
+1
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util-test.cc
File src/kudu/util/version_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util-test.cc@a48
PS3, Line 48:
:
:
:
It's nice these are no longer 'valid' version strings: they were too weird, but it was necessary to explicitly document how the prior version of the parser works, so they ended up here.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:14:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13897/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13897/2//COMMIT_MSG@11
PS2, Line 11: Kudu
> nit: upstream Kudu ?
Done
http://gerrit.cloudera.org:8080/#/c/13897/2//COMMIT_MSG@13
PS2, Line 13: the string parsing
> Yeah, I thought about SplitStringPieceToVector(), SplitStringUsing(), and
Ack
http://gerrit.cloudera.org:8080/#/c/13897/2/src/kudu/util/version_util.h
File src/kudu/util/version_util.h:
http://gerrit.cloudera.org:8080/#/c/13897/2/src/kudu/util/version_util.h@50
PS2, Line 50: // The unsignedness is a style violation, but it allows the implementation to
: // properly reject negative version numbers.
> +1
I ended up reverting this back to signed ints because I could achieve the same thing using a regex.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 23:31:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@65
PS3, Line 65: *
nit: Should be a +? Or is "1.10.0-" considered valid?
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 23:39:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
version_util: allow period as delimiter in extra and tighten version syntax
This will permit versions like <major>.<minor>.<maintenance>.<vendor_ver>,
a common pattern used by at least one Kudu vendor. The change shouldn't have
any tangible effect upstream as we've always ignored the value of 'extra'.
The string splitting wasn't cutting it for me so I rewrote the parsing to
use POSIX extended regexes. The major side effect is that we're no longer
loosey goosey with components (i.e. "1. 2 . 3" or "+1.+2.+3"); each must
be a subsequence of nothing but digits. I don't think anyone was relying on
this excessive permissiveness and I find the new code to be more readable.
Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Reviewed-on: http://gerrit.cloudera.org:8080/13897
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/version_util-test.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
3 files changed, 46 insertions(+), 24 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Andrew Wong: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] version util: allow period as delimiter for extra
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13897
to look at the new patch set (#2).
Change subject: version_util: allow period as delimiter for extra
......................................................................
version_util: allow period as delimiter for extra
This will permit versions like <major>.<minor>.<maintenance>.<vendor_ver>,
a common pattern used by at least one major Kudu vendor. This shouldn't have
any tangible effect on Kudu as we've always ignored the value of 'extra'.
I tried to reuse the string parsing but ended up with ugly looking code, so
I rewrote it as a DFA (i.e. poor man's regex).
Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
---
M src/kudu/util/version_util-test.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
3 files changed, 84 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/13897/2
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13897
to look at the new patch set (#4).
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
version_util: allow period as delimiter in extra and tighten version syntax
This will permit versions like <major>.<minor>.<maintenance>.<vendor_ver>,
a common pattern used by at least one Kudu vendor. The change shouldn't have
any tangible effect upstream as we've always ignored the value of 'extra'.
The string splitting wasn't cutting it for me so I rewrote the parsing to
use POSIX extended regexes. The major side effect is that we're no longer
loosey goosey with components (i.e. "1. 2 . 3" or "+1.+2.+3"); each must
be a subsequence of nothing but digits. I don't think anyone was relying on
this excessive permissiveness and I find the new code to be more readable.
Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
---
M src/kudu/util/version_util-test.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
3 files changed, 46 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/13897/4
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13897
to look at the new patch set (#3).
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
version_util: allow period as delimiter in extra and tighten version syntax
This will permit versions like <major>.<minor>.<maintenance>.<vendor_ver>,
a common pattern used by at least one Kudu vendor. The change shouldn't have
any tangible effect upstream as we've always ignored the value of 'extra'.
The string splitting wasn't cutting it for me so I rewrote the parsing to
use POSIX extended regexes. The major side effect is that we're no longer
loosey goosey with components (i.e. "1. 2 . 3" or "+1.+2.+3"); each must
be a subsequence of nothing but digits. I don't think anyone was relying on
this excessive permissiveness and I find the new code to be more readable.
Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
---
M src/kudu/util/version_util-test.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
3 files changed, 46 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/13897/3
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 5:
(1 comment)
> It doesn't need to be done in this change, but at some point it might be a good idea to decouple version as in artifact version.txt type version and feature flag x things is supported version.
>
> Currently we use the version in `VersionSupportsRF1Movement`, If instead we checked in a version epoch of sorts we could decouple these two concepts and avoid weird issues.
I think the issue here was that, at the time that VersionSupportsRF1Movement was added, we wanted it to be retroactively true for several releases of Kudu. A feature flag can't address that: it'll only give you "supports X"=true from the release that added the flag rather than a release or two before then.
Alexey would know more about it.
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@97
PS3, Line 97: matches[4].rm_eo - (matches[4].rm_s
> nit: I don't think it matters because we're at the end of the string anyway
Hmm, no. I'm surprised UBSAN didn't complain about undefined behavior.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:15:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@65
PS3, Line 65: *
> It's valid. The delimiter is chopped off and the 'extra' component is empty
I see.
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@97
PS3, Line 97: matches[4].rm_eo - matches[4].rm_so
nit: I don't think it matters because we're at the end of the string anyway, but was it intentional to leave off consideration for the + 1 here?
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:09:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@65
PS3, Line 65: .-]
> nit: does '.' has special meaning in the set of acceptable characters? I s
Hmm, you may be right; I removed the escaping and the unit tests still passed.
Here's what the manpage says:
To include a literal ']' in the list, make it the first character (following a possible '^').
To include a literal '-', make it the first or last character, or the second endpoint of a range.
To use a literal '-' as the first endpoint of a range, enclose it in "[." and ".]" to make it a collating element (see below).
With the exception of these and some combinations using '[' (see next paragraphs), all other special characters, including '\', lose their special significance within a bracket expression.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:08:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter for extra
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter for extra
......................................................................
Patch Set 2: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13897/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13897/2//COMMIT_MSG@11
PS2, Line 11: Kudu
nit: upstream Kudu ?
http://gerrit.cloudera.org:8080/#/c/13897/2//COMMIT_MSG@13
PS2, Line 13: the string parsing
Yeah, I thought about SplitStringPieceToVector(), SplitStringUsing(), and SplitStringAndParse from gutil/strings/split.h, but it seems the parser should be strict w.r.t. the '-' alternative separator to be only before the extra part and those functions are missing info on the delimiter they found per a parsed component.
http://gerrit.cloudera.org:8080/#/c/13897/2/src/kudu/util/version_util.h
File src/kudu/util/version_util.h:
http://gerrit.cloudera.org:8080/#/c/13897/2/src/kudu/util/version_util.h@50
PS2, Line 50: // The unsignedness is a style violation, but it allows the implementation to
: // properly reject negative version numbers.
+1
With this, the artifact of the prior implementation became explicit :)
I think the functionality is more important than the style guide in this context.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 17:30:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 3:
It doesn't need to be done in this change, but at some point it might be a good idea to decouple version as in artifact version.txt type version and feature flag x things is supported version.
Currently we use the version in `VersionSupportsRF1Movement`, If instead we checked in a version epoch of sorts we could decouple these two concepts and avoid weird issues.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:09:51 +0000
Gerrit-HasComments: No
[kudu-CR] version util: allow period as delimiter for extra
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter for extra
......................................................................
Patch Set 2:
This could probably be done more cleanly with regex.h, but need to figure out what the exact pattern should be.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 08:54:28 +0000
Gerrit-HasComments: No
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 5:
> I think the issue here was that, at the time that VersionSupportsRF1Movement was added, we wanted it to be retroactively true for several releases of Kudu. A feature flag can't address that: it'll only give you "supports X"=true from the release that added the flag rather than a release or two before then.
That's why adding a feature version of sorts now, will prevent the need to use the real version as a proxy again in the future.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:16:49 +0000
Gerrit-HasComments: No
[kudu-CR] version util: allow period as delimiter in extra and tighten version syntax
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13897 )
Change subject: version_util: allow period as delimiter in extra and tighten version syntax
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/13897/3/src/kudu/util/version_util.cc@65
PS3, Line 65: \\.
nit: does '.' has special meaning in the set of acceptable characters? I suspect the escaping might not be necessary here.
--
To view, visit http://gerrit.cloudera.org:8080/13897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5502efa8ba378d96432e530366e74a82c83621f7
Gerrit-Change-Number: 13897
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 00:03:13 +0000
Gerrit-HasComments: Yes