You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by eribeiro <gi...@git.apache.org> on 2016/12/28 17:23:38 UTC
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
GitHub user eribeiro opened a pull request:
https://github.com/apache/zookeeper/pull/137
ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eribeiro/zookeeper ZOOKEEPER-2573
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/137.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #137
----
commit c0c0daf3ebee0d280e592a0d262ffc0939f6f762
Author: Edward Ribeiro <ed...@gmail.com>
Date: 2016-12-28T17:21:31Z
ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r97247778
--- Diff: src/lastRevision.bat ---
@@ -16,8 +16,9 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem Find the current revision, store it in a file, for DOS
-svn info | findstr Revision > %1
+git rev-parse HEAD > %1
-For /F "tokens=1,2 delims= " %%a In (%1) Do (
- echo lastRevision=%%b> %1
+For /F "tokens=* delims= " %%a In (%1) Do (
--- End diff --
This is not working. May be you can replace with all the script with following script:
for /f "delims=" %%i in ('git rev-parse HEAD') do set rev=%%i
echo lastRevision=%rev% > %1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro closed the pull request at:
https://github.com/apache/zookeeper/pull/137
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/137
@rakeshadr I did a couple of changes, including the ones you suggested. Please, take a look whenever possible. Cheers!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/137
Thanks @arshadmohammad, gonna do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on the issue:
https://github.com/apache/zookeeper/pull/137
I think we should handle it in the VerGen. Can we trim and then assign the value to rev variable
As:
rev=rev.trim()
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96649084
--- Diff: src/java/main/org/apache/zookeeper/Version.java ---
@@ -20,7 +20,7 @@
public class Version implements org.apache.zookeeper.version.Info {
- public static int getRevision() {
+ public static String getRevision() {
--- End diff --
I have checked with my HBase colleagues, they said no dependency with "revision". From ZK point of view, not required to maintain backward compatibility as this is not exposed interface. Still, if any concerns, then we can deprecate and create a new GIT_REVISION or REVISION_HASH etc as this change will go to stable branch-3.4
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96560786
--- Diff: src/lastRevision.sh ---
@@ -16,6 +16,6 @@
# Find the current revision, store it in a file
FILE=$1
-LASTREV=`svn info | grep '^Revision' | sed -e 's/Revision: *//'`
+LASTREV=`git rev-parse HEAD | cut -c1-8`
--- End diff --
Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96218924
--- Diff: src/java/main/org/apache/zookeeper/Version.java ---
@@ -20,7 +20,7 @@
public class Version implements org.apache.zookeeper.version.Info {
- public static int getRevision() {
+ public static String getRevision() {
--- End diff --
Since this is not an exposed interface it is OK to change the signature. But, I'm curious to know that any other project or users depending on the revision number.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96212537
--- Diff: src/lastRevision.sh ---
@@ -16,6 +16,6 @@
# Find the current revision, store it in a file
FILE=$1
-LASTREV=`svn info | grep '^Revision' | sed -e 's/Revision: *//'`
+LASTREV=`git rev-parse HEAD | cut -c1-8`
--- End diff --
Thanks @eribeiro for the patch. Any specific reason to use only the first 8 characters of the git SHA-1. Will this be helpful in analyzing whether a particular fix included in the release?
How about keeping the complete SHA-1?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/137
@eribeiro, I have cherry picked the changes to branch-3.4 using #155 pull request. Could you please verify the branch-3.4 changes once and close this PR. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96560053
--- Diff: src/lastRevision.sh ---
@@ -16,6 +16,6 @@
# Find the current revision, store it in a file
FILE=$1
-LASTREV=`svn info | grep '^Revision' | sed -e 's/Revision: *//'`
+LASTREV=`git rev-parse HEAD | cut -c1-8`
--- End diff --
@eribeiro, could you please update the patch accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96636920
--- Diff: src/java/main/org/apache/zookeeper/Version.java ---
@@ -20,7 +20,7 @@
public class Version implements org.apache.zookeeper.version.Info {
- public static int getRevision() {
+ public static String getRevision() {
--- End diff --
@rakeshadr My Google searches didn't bring anything about external ZK revision number use. Of course, many in-house systems can rely on it. So, I am clueless about the best strategy here. :(
It would be quite annoying to break internal build tools, but this is an unfortunate side effect of a fundamental infra change (VCS). :(
@fpj wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96225530
--- Diff: src/lastRevision.sh ---
@@ -16,6 +16,6 @@
# Find the current revision, store it in a file
FILE=$1
-LASTREV=`svn info | grep '^Revision' | sed -e 's/Revision: *//'`
+LASTREV=`git rev-parse HEAD | cut -c1-8`
--- End diff --
Hi @rakeskadr, usually the first 8 characters of commit are enough to identify the commit.
But I think putting the whole SHA-1 is nice, because the more context the better. Also, it simplifies the code even more (to substr the sha1 is annoying on windows).
Thanks for pointing out!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r97248046
--- Diff: src/lastRevision.bat ---
@@ -16,8 +16,9 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem Find the current revision, store it in a file, for DOS
-svn info | findstr Revision > %1
+git rev-parse HEAD > %1
-For /F "tokens=1,2 delims= " %%a In (%1) Do (
- echo lastRevision=%%b> %1
+For /F "tokens=* delims= " %%a In (%1) Do (
--- End diff --
Thanks @arshadmohammad!
I was hoping for some committer/contributor with access to a Windows box to help me out with the BAT script. Did you test it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/137
@eribeiro, Earlier with svn revision(integer value), if there is NFE while parsing arg[1] then it suppress this exception and returns -1. The outer try-catch block guards the from #parseVersionString() function. IIUC, that is reason for separate try-catch blocks.
Anyway, now with the git rev hash the inner try-catch block is not needed as we are dealing with String value. I could see you have removed the inner try-catch block, that looks OK to me. Still we need to maintain the outer try-catch block where it guards MAJ, MINOR, MICRO parsing, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/137
lgtm, though I don't have a windows box to test changes on lastRevision.bat.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on the issue:
https://github.com/apache/zookeeper/pull/137
Thanks @eribeiro for working on this issue.
LGTM +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt g...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/137#discussion_r96225742
--- Diff: src/java/main/org/apache/zookeeper/Version.java ---
@@ -20,7 +20,7 @@
public class Version implements org.apache.zookeeper.version.Info {
- public static int getRevision() {
+ public static String getRevision() {
--- End diff --
Good question! Um... Maybe we should contact folks at Kafka, HBase and other projects for feedback? Anyone has a clue?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/137
Thanks @arshadmohammad, @hanm for the reviews.
@eribeiro , it looks like "build.xml" changes has conflicts in branch-3.5 and master. Could you please create another pull request for committing your changes into branch-3.5 and master. I will commit both this PR and the new PR together. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/137
Right. Thanks for the explanation, @rakeshadr!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/137
+1 latest changes looks good to me. @hanm, @arshadmohammad, could you please pitch in and let me know your feedback on latest changes as you have reviewed earlier, Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/137
HI @rakeshadr, I have just created a PR targeting branch-3.5 (https://github.com/apache/zookeeper/pull/155 ). It can be applied to master too. In fact, I was able to apply PR 155 to branch-3.4 so you may try to use it instead of this one, if you prefer. Otherwise you can squash and merge this one on branch-3.4 and 155 on branch-3.5 and master.
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/137
@rakeshadr Sure! Doing it right now! Thanks! :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #137: ZOOKEEPER-2573: Modify Info.REVISION to adapt git repo
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on the issue:
https://github.com/apache/zookeeper/pull/137
I verified on windows. Other than one minor problem the current changes look good to me.
commit hash is getting added with one extra space at the end.
here is srvr command output
-------------------------------------------------------
Zookeeper version: 3.4.10-SNAPSHOT-3271f3d03d75c675ec1e466434eee0834cb9841a , built on 01/23/2017 19:11 GMT
Latency min/avg/max: 0/1/31
Received: 52
Sent: 51
Connections: 2
Outstanding: 0
Zxid: 0x1
Mode: standalone
Node count: 4
------------------------
There is space after 3271f3d03d75c675ec1e466434eee0834cb9841a
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---