feat: better mismatch detection of scan results #130

prawilny · 2021-11-01T13:42:46Z

I created a single PR as two last points in #121 would require a lot of rebasing should they need to be rewritten.
Warning: there's quite a lot of copypasted code with only ~5 lines of changes in tests in TestMirroringTable (the tests are there instead of in TestMirroringResultScanner because this way there needs to be significantly less boilerplate).

This change is

mwalkiewicz

Reviewed 2 of 16 files at r1, all commit messages.
Reviewable status: 2 of 16 files reviewed, 5 unresolved discussions (waiting on @mwalkiewicz and @prawilny)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringResultScanner.java, line 237 at r1 (raw file):

    final int permitsNeeded = orderEnforcer.getNextPermits();
    FutureCallback<T> orderEnforcedScannerNext =

This trick is neat, but I think that it is not very obvious and can be made more straightforward and more readable with a plain queue and a mutex.

I think that it'd be enough to create a queue of entries to be verified (AsyncScannerVerificationPayload and AsyncScannerExceptionWithContext produced by AsyncResultScannerWrapper#performNext() - that method should then return nulls, i think) and in this callback we take a mutex, pop an element from the queue, and perform verification on that element. That would give us ordered pairs of primary and secondary results, and a synchronization, and would be explicit about the purpose.

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringResultScanner.java, line 277 at r1 (raw file):

  }

  public class LastUnmatched {

I do not yet know how the algorithm in this class works, but I see something now:

I do not likeprimaryPreviouslyUnmatched name, I do not think that the same says what it does;
Comparison logic is now split between two files, can we make LastUnmatched just a struct with two deques, and move all the logic into static methods in MismatchDetector?

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringResultScanner.java, line 383 at r1 (raw file):

    }

    public <T> void waitForTurn(int permits) {

why <T>?

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 168 at r1 (raw file):

      Scan scan,
      HBaseOperation operation) {
    int primaryIdx = 0, secondaryIdx = 0;

Split declarations into separate lines. https://google.github.io/styleguide/javaguide.html#s4.8.2-variable-declarations

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java, line 1434 at r1 (raw file):

  @Test
  public void testUnmatchedScannerResultQueuesAreFlushedWhenResultScannerIsClosed()

I'd also test if everything works as expected when:

MIRRORING_SCANNER_BUFFERED_MISMATCHED_READS == 0;
primary and secondary return the same values at first, then primary returns a single value not in the secondary, then values match, and later primary returns exclusive result once again.
number of buffered records exceeds the threshold
number of buffered records exceeds the threshold, but later the somehow start to return the same values again (don't know if it describes any sane real-life scenario, but I'd like to know what will happen, as a sanity check)
scanner first encounters a result missing from primary, and after several reads a result missing from secondary.

prawilny

Dismissed @mwalkiewicz from 3 discussions.
Reviewable status: 0 of 22 files reviewed, 2 unresolved discussions (waiting on @mwalkiewicz)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringResultScanner.java, line 237 at r1 (raw file):