StoredTabletFile doesn't override TabletFile.equals() - spotbugs warning at rank 17

[EdColeman]

Describe the bug

Looking at the warning / errors generated when raising the spotBugs maxRank (in the spotbugs maven plug-in) from the current 16 to 17, there are a few interesting warnings about classes not overriding equals. This is one example. The warning:

ERROR: Medium: org.apache.accumulo.core.metadata.StoredTabletFile doesn't override TabletFile.equals(Object) [org.apache.accumulo.core.metadata.StoredTabletFile] At StoredTabletFile.java:[line 1] EQ_DOESNT_OVERRIDE_EQUALS

** To Replicate ** Temporally raise the spotbugs plugin error rank to 17, there will be ~33 errors in core - one of them as listed above:

       <groupId>com.github.spotbugs</groupId>
          <artifactId>spotbugs-maven-plugin</artifactId>
           ....
            <maxRank>17</maxRank>

If the code implements an equals() that just delegates the parent, it preserves the current behavior and allows the test to pass..

However, you modify equals() to include the new field in the child class (using IDE generated code, for example)

Passing code:

  @Override
  public boolean equals(Object o) {
    return super.equals(o);
  }

"Normal equals"

 @Override
  public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;
    if (!super.equals(o)) return false;
    StoredTabletFile that = (StoredTabletFile) o;
    return metadataEntry.equals(that.metadataEntry);
  }

This fails at least one test:

[ERROR]   TabletFileTest.testNormalizePath:106 expected: org.apache.accumulo.core.metadata.StoredTabletFile@5e5af8e1<hdfs://nn.somewhere.com:86753/accumulo/tables/2a/t-0003/C0004.rf> but was: org.apache.accumulo.core.metadata.StoredTabletFile@30b131b2<hdfs://nn.somewhere.com:86753/accumulo/tables/2a/t-0003/C0004.rf>

So, currently, without any modification equals defaults to the parent. Adding and delegating to the parent makes that explicit, and fixes the spotbugs warning. The question is though, is that correct? Or maybe it is just a test artifact? The description of the class implies that it is important to capture exactly what is in the metadata - shouldn't an equality check or hash code generation on this class use that same, exact information? On the surface, the child class seems to want strict copies of the metadata entries, for some things but has implicit, relaxed restrictions other cases. Not implying that it's wrong, just seems odd and undocumented.

Changing the equals() implementation does change the behavior, so equals is being used in some fashion and not just, well its never used so it does not matter.

Seeking discussion more than just committing this change.

Versions (OS, Maven, Java, and others, as appropriate): Accumulo 2.1 (main branch)

[milleruntime]

Interesting... So the test is checking that the path gets normalized but it uses 2 different objects to do the check. When you add the new equals method, it is failing because they are not equal. I think the "Normal equals" method you have is fine. The test should just be updated to check that the normalized paths are equal but the objects are not equal.

[EdColeman]

Besides the TabletFileTest, It looks like any change causes additional Test failures. Either using a "normal" equals that includes the parent equals or just using the stored path for the class.

[ERROR] Failures: 
[ERROR]   BulkIT.test execution timed out after 240000 ms
[ERROR]   ShellServerIT.importDirectory execution timed out after 60000 ms
[ERROR]   ShellServerIT.importDirectoryOld:1222 expected: <0> but was: <1>
[ERROR]   ShellServerIT.importDirectoryWithOptions execution timed out after 60000 ms
[ERROR]   ShellServerIT.testFateCommandWithSlowCompaction:2114 0 transactions present in root@miniInstance ShellServerIT_testFateCommandWithSlowCompaction0> fate -print
txid: 287ac13b954554e2  status: IN_PROGRESS         op: PrepBulkImport   locked: [R:m]           locking: []              top: LoadFiles       created: 2022-07-28T17:22:16.232Z
txid: 7bd0c58769530bb1  status: IN_PROGRESS         op: PrepBulkImport   locked: [R:1h]          locking: []              top: LoadFiles       created: 2022-07-28T17:24:48.364Z
 2 transactions
 was not true ==> expected: <true> but was: <false>
[ERROR]   ShellServerIT.testFateSummaryCommandWithSlowCompaction:2185 expected: <{}> but was: <{IN_PROGRESS=2}>

[EdColeman]

Should be handled with Update TabletFile methods #2854. The issue as found by @ctubbsii:

Bulk v2 LoadFiles reads creates a Map<TabletFile, ?> and puts StoredTabletFiles in it. Then, it calls map.contains(new TabletFile(...)) . So, it's trying to use an equality comparison between StoredTabletFiles and TabletFiles, because of the attempt to compare what was loaded from the metadata table, and what is being attempted to be bulk imported. This happens in the fate operation, which seems to retry indefinitely until .contains is eventually true. Because of this, we need to ensure the equals, hashCode, and compareTo methods all have parity, so they give the same result regardless of which method is being called.

[EdColeman]

Another approach that does not seem to resolve the problem was discussed in PR #2854

[cshannon]

I am planning to revisit this.