Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9280 )

Change subject: util: fix logged Kudu URL
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9280/3/src/kudu/util/version_info.cc
File src/kudu/util/version_info.cc:

http://gerrit.cloudera.org:8080/#/c/9280/3/src/kudu/util/version_info.cc@25
PS3, Line 25: #include <regex.h>
> I was basing the implementation on that in rpc/sasl_common.cc. A comment th
Ah I see...if it's busted then we shouldn't use it. Todd wrote that comment, so 
maybe he can tell you what's busted.


http://gerrit.cloudera.org:8080/#/c/9280/3/src/kudu/util/version_info.cc@49
PS3, Line 49: "\\([[:digit:]]\\+\\.[[:digit:]]\\+\\.[[:digit:]]\\+\\)-.*"
> I agree, I'm surprised it needed this much escaping, but in test, this is w
The crazy escaping may be the difference between the flavor of regex.h you're 
using and the default used in C++11 regex? Seems weird to me though. You also 
might be able to use C++11 raw strings to get rid of some escaping and make the 
regex more readable.


http://gerrit.cloudera.org:8080/#/c/9280/3/src/kudu/util/website_util.cc
File src/kudu/util/website_util.cc:

http://gerrit.cloudera.org:8080/#/c/9280/3/src/kudu/util/website_util.cc@37
PS3, Line 37: if (version_number.empty()) {
If I read the regex right, if the version is not of the form "x.y.z-something" 
then this will be empty- the "-something" on the end is not optional, though 
"something" could be empty. Therefore, won't we fail to give release URLs for 
e.g. 1.7.0?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I357693122d27337183cb85c677a85e7b8d63fe48
Gerrit-Change-Number: 9280
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <granthe...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 20:26:14 +0000
Gerrit-HasComments: Yes

Reply via email to