Bigtable 9b. Implement ReadRows row merging logic. #2914

igorbernstein2 · 2018-02-16T01:05:09Z

This PR sets the stage for ReadRows by implementing the row merging in a RowMergingCallable. In a future commit, this callable will be part of a chain that will implement the ReadRowsCallable chain. For now its disconnected.

The implementation relies on ReframingResponseObserver to handle flow control and integrates with it by implementing the Reframer interface in RowMerger.
The RowMerger, simply glues the Reframer api to the StateMachine.
The StateMachine contains all of the logic of merging rows.

This has not been reviewed yet by anyone on the Bigtable team.
@kevinsi4508 can you take a look at this when you have a (very long) moment?

For reference the reframing logic was merged in this PR:
#2907

This PR sets the stage for ReadRows by implementing the row merging in a RowMergingCallable. In a future commit, this callable will be part of a chain that will implement the ReadRowsCallable chain. For now its disconnected. The implementation relies on ReframingResponseObserver to handle flow control and integrates with it by implementing the Reframer interface in RowMerger. The RowMerger, simply glues the Reframer api to the StateMachine. The StateMachine contains all of the logic of merging rows.

kevinsi4508

Some comments, more to come.

...-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/ByteStringComparator.java

+      if (byteA == byteB) {
+        continue;
+      }
+      return byteA < byteB ? -1 : 1;


...-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/ByteStringComparator.java

+    if (sizeA == sizeB) {
+      return 0;
+    }
+    return sizeA < sizeB ? -1 : 1;


...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+    buffer = response;
+    nextChunk = 0;
+
+    // If the server sends a scan heartbeat, wrap it in a synthetic row that will be be filtered out


...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+    nextChunk = 0;
+
+    // If the server sends a scan heartbeat, wrap it in a synthetic row that will be be filtered out
+    // after the resume logic.


...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+
+  @Override
+  public boolean hasFullFrame() {
+    return nextRow != null || readNextRow();


...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+ * <p>Package-private for internal use.
+ */
+@InternalApi
+class RowMerger<RowT> implements Reframer<RowT, ReadRowsResponse> {


...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+
+  @Override
+  public boolean hasPartialFrame() {
+    return hasFullFrame() || stateMachine.isRowInProgress();


...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+
+  @Override
+  public RowT pop() {
+    RowT row = nextRow;


igorbernstein2 · 2018-02-20T15:45:52Z

Ok, I think I addressed all of Kevin's feedback. @pongad @garrettjonesgoogle, this should be ready for you

kevinsi4508 · 2018-02-21T13:59:54Z

LGTM, please ask Gary to take a look as well.

igorbernstein2 · 2018-02-21T14:14:07Z

@garye can you take a look as well?

...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+ *
+ * <ul>
+ *   <li>Logical rows that were constructed using the {@link RowBuilder}
+ *   <li>Special marker rows that represent resumption points that were sent by Last scanned row


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+  /**
+   * Feeds the last scanned serverside key into the state machine. The adapter will wrap this key in
+   * a special marker row that can be used downstream for efficient resume. It is the callers
+   * responsibility to eventually filter out this row.


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+
+  /**
+   * Feeds the last scanned serverside key into the state machine. The adapter will wrap this key in
+   * a special marker row that can be used downstream for efficient resume. It is the callers


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+   *   <dt>Valid states:
+   *   <dd>{@link StateMachine#AWAITING_NEW_ROW}
+   *   <dt>Resulting states:
+   *   <dd>{@link StateMachine#AWAITING_ROW_CONSUME}


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+
+  /**
+   * Returns the last completed row and transitions to awaiting a new row.
+   *


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+
+        @Override
+        State handleChunk(CellChunk chunk) {
+          validate(!chunk.getResetRow(), "AWAITING_NEW_ROW: can't reset");


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+          rowKey = chunk.getRowKey();
+          adapter.startRow(rowKey);
+
+          // auto transition


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+          }
+          adapter.finishCell();
+
+          if (!chunk.getCommitRow()) {


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+        }
+      };
+
+  private static final State AWAITING_ROW_CONSUME =


igorbernstein2 · 2018-02-21T19:44:22Z

@garye Thanks for reviewing, I think addressed all feedback. PTAL

pongad

A few nits but looks good as far as I can tell otherwise. I'm not qualified to fully review the state machine though :(

...cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/RowMerger.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.google.cloud.bigtable.data.v2.stub.read_rows;


...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/read_rows/StateMachine.java

+
+  /**
+   * Feeds a new chunk into the sate machine. If the chunk is invalid, the state machine will throw
+   * an exception and should be used for further input.


igorbernstein2 · 2018-02-21T22:37:09Z

Please merge when ready.

igorbernstein2 requested a review from pongad as a code owner February 16, 2018 01:05

googlebot added the cla: yes label Feb 16, 2018

igorbernstein2 added 5 commits February 15, 2018 20:09

fix doc

0f0efeb

cleanup the acceptance test

d460b5d

clean up chunk handling to have better flow

bbfdaa9

whitespace

1b489f1

use static

1f16c3c

googleapis deleted a comment Feb 16, 2018

kevinsi4508 reviewed Feb 16, 2018

View reviewed changes

googleapis deleted a comment Feb 17, 2018

igorbernstein2 added 2 commits February 16, 2018 23:37

annotate RowCell create() with @nonnull

3172c71

minor cleanup of read rows acceptance test

94dc506

googleapis deleted a comment Feb 17, 2018

fix doc

1f411a3

googleapis deleted a comment Feb 17, 2018

address feedback

04e5304

igorbernstein2 added 2 commits February 20, 2018 14:27

Add a couple tests for row merging not covered by the json

ae05a63

remove debug

5194407

garye suggested changes Feb 21, 2018

View reviewed changes

address feedback

e567e36

pongad reviewed Feb 21, 2018

View reviewed changes

rename read_rows -> readrows, fix docs

9ee63e6

pongad merged commit 7131ecc into googleapis:master Feb 21, 2018

igorbernstein2 deleted the 09b-impl-read-rows-merging branch February 22, 2018 11:55

igorbernstein2 mentioned this pull request Feb 22, 2018

Bigtable: 10. Implementation: integrate ReadRows #2940

Merged

Bigtable 9b. Implement ReadRows row merging logic. #2914

Bigtable 9b. Implement ReadRows row merging logic. #2914

Conversation

igorbernstein2 commented Feb 16, 2018 • edited Loading

kevinsi4508 left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

igorbernstein2 commented Feb 20, 2018

kevinsi4508 commented Feb 21, 2018

igorbernstein2 commented Feb 21, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

igorbernstein2 commented Feb 21, 2018

pongad left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

igorbernstein2 commented Feb 21, 2018

igorbernstein2 commented Feb 16, 2018 •

edited

Loading