You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "bmscomp (via GitHub)" <gi...@apache.org> on 2023/04/23 21:26:59 UTC
[GitHub] [kafka] bmscomp opened a new pull request, #13627: MINOR: simplify if else conditions in Uuid.compareTo
bmscomp opened a new pull request, #13627:
URL: https://github.com/apache/kafka/pull/13627
Replace `if else ` conditions with `Long.compare` for reducing verbosity
### Committer Checklist (excluded from commit message)
- [] Verify design and implementation
- [] Verify test coverage and CI build status
- [] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1178371488
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -143,12 +143,6 @@ public int compareTo(Uuid other) {
return 1;
Review Comment:
@divijvaidya Still there is another way to implement the `compareTo` method, and by the way `compare` can be a pretty name also for the method, and it will keep the same style of the compare method name in `Long`
if (mostSignificantBits == other.mostSignificantBits)
return Long.compare(leastSignificantBits, other.leastSignificantBits);
return Long.compare(mostSignificantBits, other.mostSignificantBits);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1176447810
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -143,12 +143,6 @@ public int compareTo(Uuid other) {
return 1;
Review Comment:
Something like this:
```
@Override
public int compareTo(Uuid other) {
int res = Long.compare(mostSignificantBits, other.mostSignificantBits);
return (res != 0) ? res : Long.compare(leastSignificantBits, other.leastSignificantBits);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13627:
URL: https://github.com/apache/kafka/pull/13627#issuecomment-1627838038
@divijvaidya Sorry for this delay, I just rebased the branch right now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1176384671
##########
.gitignore:
##########
@@ -35,6 +35,7 @@ Vagrantfile.local
config/server-*
config/zookeeper-*
core/data/*
+core/data2/*
Review Comment:
@divijvaidya Yes it comes from this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1178371488
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -143,12 +143,6 @@ public int compareTo(Uuid other) {
return 1;
Review Comment:
Still there is another way to implement the `compareTo` method, and by the way `compare` can be a pretty name also for the method, and it will keep the same style of the compare method name in `Long`
if (mostSignificantBits == other.mostSignificantBits)
return Long.compare(leastSignificantBits, other.leastSignificantBits);
return Long.compare(mostSignificantBits, other.mostSignificantBits);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13627:
URL: https://github.com/apache/kafka/pull/13627#issuecomment-1591667600
@bmscomp a lot of tests are failing right. Can you please rebase with latest trunk and we can actively try to merge this in next few days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13627:
URL: https://github.com/apache/kafka/pull/13627#issuecomment-1728953825
@divijvaidya I think it's better to stick to the same implementation as JDK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1176249165
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -143,12 +143,6 @@ public int compareTo(Uuid other) {
return 1;
Review Comment:
Can we use `Long.compare()` for mostSignificantBits as well?
##########
.gitignore:
##########
@@ -35,6 +35,7 @@ Vagrantfile.local
config/server-*
config/zookeeper-*
core/data/*
+core/data2/*
Review Comment:
perhaps add a note that this is created by a test [1], other wise other reviewers may wonder (like me) as to where is this directory coming from
https://github.com/apache/kafka/blob/trunk/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala#L2673
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13627:
URL: https://github.com/apache/kafka/pull/13627#issuecomment-1640038761
I am apprehensive about making this change. Even JDK's implementation of UUID.compareTo() uses existing code:
```
@Override
public int compareTo(UUID val) {
// The ordering is intentionally set up so that the UUIDs
// can simply be numerically compared as two numbers
return (this.mostSigBits < val.mostSigBits ? -1 :
(this.mostSigBits > val.mostSigBits ? 1 :
(this.leastSigBits < val.leastSigBits ? -1 :
(this.leastSigBits > val.leastSigBits ? 1 :
0))));
}
```
My main concern is that the comparison is dependent on treating unsigned vs. signed values. for example, why not use Long.compareUnsigned instead of using Long.compare.
I would say, it's probably safer to stick to what JDK already does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13627:
URL: https://github.com/apache/kafka/pull/13627#issuecomment-1640164474
@divijvaidya This is the implementation of Long.compare, witch expresses the same, as existing implementation
```
Long ...
public static int compare(long x, long y) {
return (x < y) ? -1 : ((x == y) ? 0 : 1);
}
```
I agree with you `Long.compareUnsigned ` is better in this case I'll update this pull request to use it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13627:
URL: https://github.com/apache/kafka/pull/13627#issuecomment-1640250647
@divijvaidya the latest version of compare in jdk uses `Long.compare`
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/UUID.java#L555
```
@Override
public int compareTo(UUID val) {
// The ordering is intentionally set up so that the UUIDs
// can simply be numerically compared as two numbers
int mostSigBits = Long.compare(this.mostSigBits, val.mostSigBits);
return mostSigBits != 0 ? mostSigBits : Long.compare(this.leastSigBits, val.leastSigBits);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1176409358
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -143,12 +143,6 @@ public int compareTo(Uuid other) {
return 1;
Review Comment:
@divijvaidya I could not find a prettier way to use `Long.compare` for `mostSignificantBits` , do you have a trick for it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on a diff in pull request #13627: MINOR: simplify if else conditions in Uuid.compareTo method
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13627:
URL: https://github.com/apache/kafka/pull/13627#discussion_r1176527874
##########
clients/src/main/java/org/apache/kafka/common/Uuid.java:
##########
@@ -143,12 +143,6 @@ public int compareTo(Uuid other) {
return 1;
Review Comment:
Thanks so much @divijvaidya, looks great, I already updated the pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org