You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/11/04 21:31:04 UTC
[GitHub] [zookeeper] belugabehr opened a new pull request #1522: ZOOKEEPER-3993: Review some hashcode / equals methods
belugabehr opened a new pull request #1522:
URL: https://github.com/apache/zookeeper/pull/1522
Was just reviewing some code and thought this could be simplified a bit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] belugabehr commented on a change in pull request #1522: ZOOKEEPER-3993: Review some hashcode / equals methods
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1522:
URL: https://github.com/apache/zookeeper/pull/1522#discussion_r634760944
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -834,30 +834,31 @@ public int aclCacheSize() {
public List<ProcessTxnResult> multiResult;
- /**
- * Equality is defined as the clientId and the cxid being the same. This
- * allows us to use hash tables to track completion of transactions.
- *
- * @see java.lang.Object#equals(java.lang.Object)
- */
@Override
- public boolean equals(Object o) {
- if (o instanceof ProcessTxnResult) {
- ProcessTxnResult other = (ProcessTxnResult) o;
- return other.clientId == clientId && other.cxid == cxid;
- }
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof ProcessTxnResult)) {
return false;
+ }
+ ProcessTxnResult other = (ProcessTxnResult) obj;
+ if (clientId != other.clientId) {
+ return false;
+ }
+ if (cxid != other.cxid) {
+ return false;
+ }
+ return true;
Review comment:
OK. Reverted.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1522: ZOOKEEPER-3993: Review some hashcode / equals methods
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1522:
URL: https://github.com/apache/zookeeper/pull/1522#discussion_r517649775
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -834,30 +834,31 @@ public int aclCacheSize() {
public List<ProcessTxnResult> multiResult;
- /**
- * Equality is defined as the clientId and the cxid being the same. This
- * allows us to use hash tables to track completion of transactions.
- *
- * @see java.lang.Object#equals(java.lang.Object)
- */
@Override
- public boolean equals(Object o) {
- if (o instanceof ProcessTxnResult) {
- ProcessTxnResult other = (ProcessTxnResult) o;
- return other.clientId == clientId && other.cxid == cxid;
- }
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof ProcessTxnResult)) {
return false;
+ }
+ ProcessTxnResult other = (ProcessTxnResult) obj;
+ if (clientId != other.clientId) {
+ return false;
+ }
+ if (cxid != other.cxid) {
+ return false;
+ }
+ return true;
}
- /**
- * See equals() to find the rational for how this hashcode is generated.
- *
- * @see ProcessTxnResult#equals(Object)
- * @see java.lang.Object#hashCode()
- */
@Override
public int hashCode() {
- return (int) ((clientId ^ cxid) % Integer.MAX_VALUE);
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + (int) (clientId ^ (clientId >>> 32));
+ result = prime * result + cxid;
+ return result;
Review comment:
Why not something simple, like `Long.hashCode(clientId + cxid)`?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -834,30 +834,31 @@ public int aclCacheSize() {
public List<ProcessTxnResult> multiResult;
- /**
- * Equality is defined as the clientId and the cxid being the same. This
- * allows us to use hash tables to track completion of transactions.
- *
- * @see java.lang.Object#equals(java.lang.Object)
- */
@Override
- public boolean equals(Object o) {
- if (o instanceof ProcessTxnResult) {
- ProcessTxnResult other = (ProcessTxnResult) o;
- return other.clientId == clientId && other.cxid == cxid;
- }
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof ProcessTxnResult)) {
return false;
+ }
+ ProcessTxnResult other = (ProcessTxnResult) obj;
+ if (clientId != other.clientId) {
+ return false;
+ }
+ if (cxid != other.cxid) {
+ return false;
+ }
+ return true;
Review comment:
Aside from the same object short circuit, I think the old check that had `a == o.a && b == o.b` was easier to read than all the extra `if` blocks and `!=` comparisons.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] belugabehr commented on a change in pull request #1522: ZOOKEEPER-3993: Review some hashcode / equals methods
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1522:
URL: https://github.com/apache/zookeeper/pull/1522#discussion_r634761879
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -834,30 +834,31 @@ public int aclCacheSize() {
public List<ProcessTxnResult> multiResult;
- /**
- * Equality is defined as the clientId and the cxid being the same. This
- * allows us to use hash tables to track completion of transactions.
- *
- * @see java.lang.Object#equals(java.lang.Object)
- */
@Override
- public boolean equals(Object o) {
- if (o instanceof ProcessTxnResult) {
- ProcessTxnResult other = (ProcessTxnResult) o;
- return other.clientId == clientId && other.cxid == cxid;
- }
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (!(obj instanceof ProcessTxnResult)) {
return false;
+ }
+ ProcessTxnResult other = (ProcessTxnResult) obj;
+ if (clientId != other.clientId) {
+ return false;
+ }
+ if (cxid != other.cxid) {
+ return false;
+ }
+ return true;
}
- /**
- * See equals() to find the rational for how this hashcode is generated.
- *
- * @see ProcessTxnResult#equals(Object)
- * @see java.lang.Object#hashCode()
- */
@Override
public int hashCode() {
- return (int) ((clientId ^ cxid) % Integer.MAX_VALUE);
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + (int) (clientId ^ (clientId >>> 32));
+ result = prime * result + cxid;
+ return result;
Review comment:
This proposed solution is not great for collisions, since the two values will collide (For example):
clientId = 100
cxid = 0
clientId = 99
cxid = 1
Better to treat them individually. I did simplify with Long#hashCode
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org