Fix Manifest/Resource equals/hashCode and add errorprone#77
Fix Manifest/Resource equals/hashCode and add errorprone#77ctubbsii merged 4 commits intoapache:mainfrom
Conversation
ctubbsii
commented
Feb 20, 2026
ctubbsii
commented
- Add errorprone CI checks to GitHub Actions and compat checks for Accumulo 4.0
- Update code to fix items caught by errorprone
- Rename manifests field to monitoredManifests, just to make it more obvious that it is tracking the manifests currently being monitored
- Fix equals and hashCode methods for Manifest and Resource, to account for comment and algorithm fields, respectively
- Add test for deduplication of resources in a json
* Add errorprone CI checks to GitHub Actions and compat checks for Accumulo 4.0 * Update code to fix items caught by errorprone * Rename manifests field to monitoredManifests, just to make it more obvious that it is tracking the manifests currently being monitored * Fix equals and hashCode methods for Manifest and Resource, to account for comment and algorithm fields, respectively * Add test for deduplication of resources in a json
|
I expect the 4.0.0-SNAPSHOT test to fail for now, because the latest SNAPSHOT hasn't yet been built, but it should work locally if you build and install 4.0.0-SNAPSHOT to your local maven repository. EDIT: reran failed check after publishing latest 4.0 snapshot and it passed |
| json.append("{'location': 'file:/home/user/ClassLoaderTestA/" + i + ".jar'").append(COMMA); | ||
| json.append("'algorithm': 'MOCK',").append("'checksum': '" + i + "'}"); | ||
| var n = i; | ||
| if (withResourceCount == 10) { |
There was a problem hiding this comment.
When reading the new test testDeserializationWithDeduplication() it was confusing at first how it was duplicating. Then saw that 10 was a special value in this code.
There was a problem hiding this comment.
The special value was the simplest implementation I could think of that required the least amount of new test code, but I tried to add a comment in both places: where the value is used and where it is checked. It looks like I forgot to keep the comment in the place where it is used.
| Manifest other = (Manifest) obj; | ||
| return monitorIntervalSeconds == other.monitorIntervalSeconds | ||
| && Objects.equals(resources, other.resources); | ||
| return obj instanceof Manifest ? Objects.equals(toJson(), ((Manifest) obj).toJson()) : false; |
There was a problem hiding this comment.
Its not a problem to call Object.equals here, but does not seem needed since the json values will not be null.
There was a problem hiding this comment.
Yeah I thought about simplifying this but I liked the way it looks, so I didn't bother to change it. I don't feel strongly either way.
| LOG.trace("Skipped resource {} while another process or thread is downloading it", | ||
| resource.getLocation()); | ||
| waitingOnOtherDownloadsCount++; | ||
| continue; |
There was a problem hiding this comment.
why remove this? It will cause this process to wait instead of immediately moving on to the next resource.
There was a problem hiding this comment.
The continue was for the for loop, which ends immediately after the continue, not the do-while loop that has the wait. So, errorprone correctly flagged it as redundant.
3 participants
