[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 5: This change does break binary compatibility. We haven't broken it for common public APIs in the past. However, I am not sure if that should explicitly define our future. I am not sure the exact impact for users and how they use the client. -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 5 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 11 Jul 2019 14:30:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Brian McDevitt, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12949 to look at the new patch set (#4). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. Changed the add and set method return types from void to PartialRow so that the method calls can be chained. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/4 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Brian McDevitt, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12949 to look at the new patch set (#3). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. -Changed PartialRow return types from void to PartialRow so that the method calls can be chained. -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. -Removed redundant indentation. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/3 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12949/3//COMMIT_MSG Commit Message: PS3: Some of the lines in this commit message are too long. Please wrap lines at 72, 76, or 80 characters (your choice). http://gerrit.cloudera.org:8080/#/c/12949/3//COMMIT_MSG@10 PS3, Line 10: -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. : -Removed redundant indentation. These two reflect changes made to your original patch in response to code review, right? You don't need to list them here; ultimately this commit message is going to be merged into the source repository as-is and anyone looking at this commit will only see the change it made, not all of its intermediate states in code review. -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Apr 2019 20:38:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Brian McDevitt, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12949 to look at the new patch set (#2). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. -Changed PartialRow return types from void to PartialRow so that the method calls can be chained. -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. -Removed redundant indentation. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/2 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
raym...@phdata.io has abandoned this change. ( http://gerrit.cloudera.org:8080/12952 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Abandoned Abandoning redundant change. -- To view, visit http://gerrit.cloudera.org:8080/12952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I87b26e775075a0ce16da925df74240fef305f47a Gerrit-Change-Number: 12952 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 1: > Agreed. Is there something off the shelf we can use here? Yeah, the most common tool I have seen is https://lvc.github.io/japi-compliance-checker/ It actually looks like we have a script to use/run it but it's a bit outdated/broken. I will update it and run some reports. Once done I will report back here. -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Apr 2019 01:09:22 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12952 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 2: Seems like you uploaded two changes for the same patch: this change, and https://gerrit.cloudera.org/c/12949. Since we've started to review the other one, could you abandon this one and roll any delta into the other? -- To view, visit http://gerrit.cloudera.org:8080/12952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I87b26e775075a0ce16da925df74240fef305f47a Gerrit-Change-Number: 12952 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Tue, 09 Apr 2019 04:57:43 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 1: (2 comments) > Changing a void method to return a value would be source compatible but not > binary compatible. I am not sure if we have a policy on breaking binary > compatibility in the client. In the C++ client we care deeply about preserving ABI compatibility. Our contract is: if we break ABI or API compatibility in the public API surface, we must rev the C++ client library's SOVERSION. This hasn't happened yet. In the Java ecosystem I think there's a stronger culture of rebuilding applications from source when revving dependencies. Client JARs specifically are easier to deal with, as they are typically bundled with applications rather than with servers, so if there's going to be a version mismatch it's usually between the client and the server rather than the client and the application. Anyway, I think it's worth investigating whether we've broken ABI compatibility before, as we can use that to inform this decision. > If we are concerned about breaking binary compatibility we should probably > setup a tool to check for breakage between releases. Agreed. Is there something off the shelf we can use here? http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java: http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1346 PS1, Line 1346: return this; There's an extra space here between 'return' and 'this'. http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java: http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java@471 PS1, Line 471: There are two spaces here. -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 08 Apr 2019 23:47:40 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12952 to look at the new patch set (#2). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. -Changed PartialRow return types from void to PartialRow so that the method calls can be chained. -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. -Removed redundant indentation. [metrics] Remove incorrect comments Data store in std::map with key type 'const char*' is not in alphabetical order, and it's not needed to sort metrics. Change-Id: Id8788c38b73c623616b3fb2b73fcb6973df4ec87 Reviewed-on: http://gerrit.cloudera.org:8080/12921 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo KUDU-2689: Made PartialRow setters use a fluent-style. Changed PartialRow return types from void to PartialRow so that the method calls can be chained. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 KUDU-2689: Updated the function documentations with a better description of the return type. Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. Change-Id: I87b26e775075a0ce16da925df74240fef305f47a --- A java/kudu-client/derby.log M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java M src/kudu/util/metrics.cc 4 files changed, 156 insertions(+), 97 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/12952/2 -- To view, visit http://gerrit.cloudera.org:8080/12952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I87b26e775075a0ce16da925df74240fef305f47a Gerrit-Change-Number: 12952 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/12949 )
Change subject: KUDU-2689: Made PartialRow setters use a fluent-style.
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java:
http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@118
PS1, Line 118:* @return a PartialRow
Here and below. "this PartialRow"
It's important because it clearly indicates it's this row mutated, as opposed
to a new PartialRow.
http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java:
http://gerrit.cloudera.org:8080/#/c/12949/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java@411
PS1, Line 411: .addShort("int16", (short) 43)
Nit: I think this is too indented. Should be 4 spaces.
--
To view, visit http://gerrit.cloudera.org:8080/12949
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206
Gerrit-Change-Number: 12949
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Brian McDevitt
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Sat, 06 Apr 2019 21:33:10 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 1: Changing a void method to return a value would be source compatible but not binary compatible. I am not sure if we have a policy on breaking binary compatibility in the client. If we are concerned about breaking binary compatibility we should probably setup a tool to check for breakage between releases. -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 06 Apr 2019 21:19:11 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 1: Is this ABI-compatible? (do we have a documented ABI-compatibility guarantee for the Java client?) -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 06 Apr 2019 03:37:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
raym...@phdata.io has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12949 Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. Changed PartialRow return types from void to PartialRow so that the method calls can be chained. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/1 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward
