You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/06/15 14:26:21 UTC
[GitHub] [lucene-solr] ErickErickson opened a new pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
ErickErickson opened a new pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579
I've created new hashCode methods for all of the classes that implement equals but not hashCode and removed associated SuppressWarnings. I've marked certain of them with TODOs where I'm really uncertain what the right thing to do is to draw attention, but I'd appreciate people looking at the others.
gw check succeeds, but that's all I'm guaranteeing at present. I need to let this bake a while and come back and re-visit them in detail if people who know the particular code better don't give a thumbs-up. hashCode implementations can be tricky, and these are definitely straw-man.
I'll add some more comments on the JIRA
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445252504
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
WDYT about just version here? If we include files, computing the hashcode seems like it could get unreasonably expensive. It's not clear to me how long that list could be.
Comparing two different packages that have the same version would break equals too so this seems safe.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445252535
##########
File path: solr/core/src/java/org/apache/solr/cloud/rule/Rule.java
##########
@@ -365,7 +365,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(name, fuzzy);
Review comment:
Just using name here.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445346816
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
I believe that when `hashCode()` is called (when using a collection) it is followed by a call to `equals()` if there's an element in that slot. More efficient (better spreading) `hashCode()` would save calls to `equals()`. Therefore, I'm not sure saving on execution time on `hashCode()` makes sense, as we will have to pay it in the cost of computing more `equals()`.
It might pay in cases where the collections will contain very few elements, in which case `hashCode()` is not really important, but in these cases its performance is likely not an issue either.
The approach I would take here is do the right thing in implementing `hashCode()`, then if we have performance issues with some objects, understand what those are and solve them (_insert Donald Knuth quote here_).
Therefore I would base `hashCode()` on version and files like `equals()`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440281749
##########
File path: solr/core/src/java/org/apache/solr/cloud/rule/Rule.java
##########
@@ -365,7 +365,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(name, fuzzy);
Review comment:
Given that `fuzzy` is not included in the `equals()` implementation just above, we could have two instances that are `equals()` but that have different values for `hashCode()`. I believe this is not correct.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#issuecomment-649582539
I'll merge this in a bit via another methodl
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440284972
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
`equals()` above does not compare `manifestSHA512`.
Unless `manifestSHA512` are equal if `version` are (in which case no need to add it here?), we could have two objets that are `equals()` yet have different `hashCode()`, which would be against the spec (see `java.lang.Object.hashCode()` javadoc).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440306014
##########
File path: solr/solrj/src/java/org/apache/solr/common/util/ValidatingJsonMap.java
##########
@@ -348,10 +348,12 @@ public boolean equals(Object that) {
return that instanceof Map && this.delegate.equals(that);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented ValidatingJsonMap.hashCode");
-// }
+ //TODO: Really uncertain about this. Hashing the map itself seems
+ // about as expensive as resolving with equals.
Review comment:
`hashCode()` and `equals()` are not used for the same type of computations, it's not one _or_ the other, so their relative costs are not necessarily an implementation criteria.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440291350
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java
##########
@@ -288,10 +287,11 @@ public boolean equals(Object o) {
return getClusterPreferences().equals(policy.getClusterPreferences());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ //TODO: Uncertain about this one
+ @Override
+ public int hashCode() {
+ return Objects.hash(zkVersion);
Review comment:
`equals()` compares `policies`, `clusterPolicy` and `clusterPreferences`, but does not compare `zkVersion`.
`hashCode()` should not use `zkVersion` in hash computation.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445261508
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java
##########
@@ -288,10 +287,11 @@ public boolean equals(Object o) {
return getClusterPreferences().equals(policy.getClusterPreferences());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ //TODO: Uncertain about this one
+ @Override
+ public int hashCode() {
+ return Objects.hash(zkVersion);
Review comment:
Done. In the interests of speed, would just returning just the hash of getPolicies(). Even though in this case that's just the member var, future-proofing if that changes seems wise.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445269392
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java
##########
@@ -491,6 +490,11 @@ public boolean equals(Object o) {
return this == o;
}
+ @Override
Review comment:
OK, thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440302683
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java
##########
@@ -153,10 +152,12 @@ public boolean equals(Object o) {
return name.equals(replica.name);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented Replica.hashCode()");
-// }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, nodeName, collection);
Review comment:
`equals()` does not compare `nodeName` and `collection`, so `hashCode()` should not be based on these values.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440301532
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -383,10 +382,12 @@ public boolean equals(Object that) {
return super.equals(that) && Objects.equals(this.name, other.name) && this.znodeVersion == other.znodeVersion;
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented DocCollection.hashCode");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(super.hashCode(), znodeVersion, name, replicationFactor,
+ numNrtReplicas, numTlogReplicas, numPullReplicas, maxShardsPerNode,
+ autoAddReplicas, policy, readOnly);
Review comment:
The superclass (`ZkNodeProps`) `hashCode()` hashes the `props` map, so already takes care of producing a hash value based on `replicationFactor`, `numNrtReplicas`, `numTlogReplicas`, `numPullReplicas`, `maxShardsPerNode`, `autoAddReplicas`, `policy` and `readOnly` that were extracted from the map. These do not have to be added here again but it's **not** incorrect to have them.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445358817
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
What I wrote above might not hold. Given that `equals()` does:
` return Objects.equals(this.version, that.version)
&& Objects.equals(this.files, that.files);
`
it is not computing the equality of `files` when `version` is different, so indeed it might be better to not use `files` in `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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440301532
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -383,10 +382,12 @@ public boolean equals(Object that) {
return super.equals(that) && Objects.equals(this.name, other.name) && this.znodeVersion == other.znodeVersion;
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented DocCollection.hashCode");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(super.hashCode(), znodeVersion, name, replicationFactor,
+ numNrtReplicas, numTlogReplicas, numPullReplicas, maxShardsPerNode,
+ autoAddReplicas, policy, readOnly);
Review comment:
The superclass (`ZkNodeProps`) `hashCode()` hashes (Edit: in this PR is returns 0, but it should instead return the map hash value) the `props` map, so already takes care of producing a hash value based on `replicationFactor`, `numNrtReplicas`, `numTlogReplicas`, `numPullReplicas`, `maxShardsPerNode`, `autoAddReplicas`, `policy` and `readOnly` that were extracted from the map. These do not have to be added here again but it's **not** incorrect to have them.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445263481
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java
##########
@@ -153,10 +152,12 @@ public boolean equals(Object o) {
return name.equals(replica.name);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented Replica.hashCode()");
-// }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, nodeName, collection);
Review comment:
I'll use just name.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r441907028
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java
##########
@@ -491,6 +490,11 @@ public boolean equals(Object o) {
return this == o;
}
+ @Override
Review comment:
As someone who has at least a passing understanding of this code, this and the other Streaming class implementations below gets my +1.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445357108
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -587,11 +585,11 @@ public boolean equals(Object o) {
if (!getTriggerListenerConfigs().equals(that.getTriggerListenerConfigs())) return false;
return getProperties().equals(that.getProperties());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(policy, zkVersion);
Review comment:
`policy` should IMO be a final member, and when it is passed as `null` should be set to the appropriate value in the constructor like what `getPolicy()` currently does (then `getPolicy()` changed into a simple getter).
Otherwise I agree, `hashCode()` should use `getPolicy()` and not `policy` directly.
There might be a performance impact (good or bad is unclear) to also base `hashCode()` on the other members `equals()` is based on...
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445263282
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -383,10 +382,12 @@ public boolean equals(Object that) {
return super.equals(that) && Objects.equals(this.name, other.name) && this.znodeVersion == other.znodeVersion;
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented DocCollection.hashCode");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(super.hashCode(), znodeVersion, name, replicationFactor,
+ numNrtReplicas, numTlogReplicas, numPullReplicas, maxShardsPerNode,
+ autoAddReplicas, policy, readOnly);
Review comment:
Fixed zkNodeProps to return the hash of the propmap. I think for this, the name and znodeversion are enough (not even adding returning super.hashCode) in the interests of speed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445361406
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java
##########
@@ -288,10 +287,11 @@ public boolean equals(Object o) {
return getClusterPreferences().equals(policy.getClusterPreferences());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ //TODO: Uncertain about this one
+ @Override
+ public int hashCode() {
+ return Objects.hash(zkVersion);
Review comment:
Agreed. Given that `equals()` calls the getter, wise to do the same in `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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445349212
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -305,10 +303,10 @@ public boolean equals(Object o) {
return properties.equals(that.properties);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, actionClass);
Review comment:
`name` comes from `properties` so would be ok to add it to the `hashCode()` computation, but it would not be useful if `properties` is already used.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445257281
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -305,10 +303,10 @@ public boolean equals(Object o) {
return properties.equals(that.properties);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, actionClass);
Review comment:
equals doesn't even include name, so I guess it'll just have to be properties only to conform to the contract.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445576784
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -587,11 +585,11 @@ public boolean equals(Object o) {
if (!getTriggerListenerConfigs().equals(that.getTriggerListenerConfigs())) return false;
return getProperties().equals(that.getProperties());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(policy, zkVersion);
Review comment:
Yeah, thought about including the other props, but I think we're OK with just getPolicy().
Like I mentioned before, though, I'm trying to limit scope here so I'm going to leave the getPolicy stuff as it is.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445263758
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java
##########
@@ -171,8 +167,10 @@ public boolean getBool(String key, boolean b) {
public boolean equals(Object that) {
return that instanceof ZkNodeProps && ((ZkNodeProps)that).propMap.equals(this.propMap);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented ZkNodeProps.hashCode");
-// }
+
+ //TODO: I'm very uncertain about this
+ @Override
+ public int hashCode() {
+ return 0;
Review comment:
Yeah, it was late that night ;). Hashed propMap.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445260567
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -587,11 +585,11 @@ public boolean equals(Object o) {
if (!getTriggerListenerConfigs().equals(that.getTriggerListenerConfigs())) return false;
return getProperties().equals(that.getProperties());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(policy, zkVersion);
Review comment:
Looks like this just needs to be a hash on getPolicy(). I don't think it can even be the member var policy since getPolicy can change 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440304468
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java
##########
@@ -171,8 +167,10 @@ public boolean getBool(String key, boolean b) {
public boolean equals(Object that) {
return that instanceof ZkNodeProps && ((ZkNodeProps)that).propMap.equals(this.propMap);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented ZkNodeProps.hashCode");
-// }
+
+ //TODO: I'm very uncertain about this
+ @Override
+ public int hashCode() {
+ return 0;
Review comment:
A constant `hashCode()` return value is correct but does not _spread_ objects into buckets in any useful way. Here we could return `propMap.hashCode()` instead.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445358817
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
What I wrote above might not hold. Given that `equals()` does:
` return Objects.equals(this.version, that.version)
&& Objects.equals(this.files, that.files);
`
it is not computing the equality of `files` when `version` is different, so indeed it might be better to not use `files` in `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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440289119
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -587,11 +585,11 @@ public boolean equals(Object o) {
if (!getTriggerListenerConfigs().equals(that.getTriggerListenerConfigs())) return false;
return getProperties().equals(that.getProperties());
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(policy, zkVersion);
Review comment:
`equals()` does not compare `zkVersion`, so `hashCode()` should not be based on it either.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445578718
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
Thanks,
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r440287813
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
##########
@@ -305,10 +303,10 @@ public boolean equals(Object o) {
return properties.equals(that.properties);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented");
-// }
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, actionClass);
Review comment:
`equals()` is based on comparing `properties` and `hashCode()` on comparing `name` and `actionClass`. This can't guarantee two objets that are `equals()` are going to have the same `hashCode()`, as required.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445346816
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -211,7 +211,7 @@ public boolean equals(Object obj) {
@Override
public int hashCode() {
- throw new UnsupportedOperationException("TODO unimplemented");
+ return Objects.hash(version, manifestSHA512);
Review comment:
I believe that when `hashCode()` is called (when using a collection) it is followed by a call to `equals()` if there's an element in that slot. More efficient (better spreading) `hashCode()` would save calls to `equals()`. Therefore, I'm not sure saving on execution time on `hashCode()` makes sense, as we will have to pay it in the cost of computing more `equals()`.
It might pay in cases where the collections will contain very few elements, in which case `hashCode()` is not really important, but in these cases its performance is likely not an issue either.
The approach I would take here is do the right thing in implementing `hashCode()`, then if we have performance issues with some objects, understand what those are and solve them (_insert Donald Knuth quote here_).
Therefore I would base `hashCode()` on `version` and `files` like `equals()`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579#discussion_r445264830
##########
File path: solr/solrj/src/java/org/apache/solr/common/util/ValidatingJsonMap.java
##########
@@ -348,10 +348,12 @@ public boolean equals(Object that) {
return that instanceof Map && this.delegate.equals(that);
}
-// @Override
-// public int hashCode() {
-// throw new UnsupportedOperationException("TODO unimplemented ValidatingJsonMap.hashCode");
-// }
+ //TODO: Really uncertain about this. Hashing the map itself seems
+ // about as expensive as resolving with equals.
Review comment:
Right. Still, every time a hashcode is computed, it iterates the entire map which could be arbitrarily large. I don't see much other choice though.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson closed pull request #1579: SOLR-14541: Ensure classes that implement equals implement hashCode or suppress warnings
Posted by GitBox <gi...@apache.org>.
ErickErickson closed pull request #1579:
URL: https://github.com/apache/lucene-solr/pull/1579
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org