chore(bigtable): Address conformance test failures (#30754)

NoRetry_OutOfOrderError
Retry_LastScannerRow

Author: aandreassa

google-cloud-bigtable/conformance/known_failures.txt

@@ -1,7 +1,41 @@
 TestCheckAndMutateRow_Generic_CloseClient
 TestCheckAndMutateRow_Generic_DeadlineExceeded
+TestExecuteQuery_BatchesTest
+TestExecuteQuery_ChecksumMismatch
+TestExecuteQuery_ChunkingTest
+TestExecuteQuery_CloseClient
+TestExecuteQuery_ConcurrentRequests
 TestExecuteQuery_EmptyResponse
+TestExecuteQuery_ExecuteQueryRespectsDeadline
+TestExecuteQuery_FailsOnEmptyMetadata
+TestExecuteQuery_FailsOnExecuteQueryMetadata
+TestExecuteQuery_FailsOnInvalidType
+TestExecuteQuery_FailsOnNotEnoughData
+TestExecuteQuery_FailsOnNotEnoughDataWithCompleteRows
+TestExecuteQuery_FailsOnStructMissingField
+TestExecuteQuery_FailsOnSuccesfulStreamWithNoToken
+TestExecuteQuery_FailsOnTypeMismatch
+TestExecuteQuery_FailsOnTypeMismatchWithinArray
+TestExecuteQuery_FailsOnTypeMismatchWithinMap
+TestExecuteQuery_FailsOnTypeMismatchWithinStruct
+TestExecuteQuery_HeadersAreSet
+TestExecuteQuery_MapAllowsDuplicateKey
+TestExecuteQuery_NestedNullsTest
+TestExecuteQuery_NullsTest
+TestExecuteQuery_PlanRefresh
+TestExecuteQuery_PrepareQueryRespectsDeadline
+TestExecuteQuery_QueryParams
+TestExecuteQuery_RetryTest_ErrorAfterFinalData
+TestExecuteQuery_RetryTest_FirstResponse
+TestExecuteQuery_RetryTest_MidStream
+TestExecuteQuery_RetryTest_ResetCompleteBatch
+TestExecuteQuery_RetryTest_ResetPartialBatch
+TestExecuteQuery_RetryTest_TokenWithoutData
+TestExecuteQuery_RetryTest_WithPlanRefresh
 TestExecuteQuery_SingleSimpleRow
+TestExecuteQuery_StructWithDuplicateColumnNames
+TestExecuteQuery_StructWithNoColumnNames
+TestExecuteQuery_TypesTest
 TestMutateRow_Generic_DeadlineExceeded
 TestMutateRow_Generic_CloseClient
 TestMutateRows_Generic_DeadlineExceeded
@@ -15,10 +49,8 @@ TestReadRow_Generic_DeadlineExceeded
 TestReadRow_Generic_CloseClient
 TestReadRow_Retry_WithRoutingCookie
 TestReadRow_Retry_WithRetryInfo
-TestReadRows_NoRetry_OutOfOrderError
 TestReadRows_NoRetry_OutOfOrderError_Reverse
 TestReadRows_ReverseScans_FeatureFlag_Enabled
-TestReadRows_Retry_LastScannedRow
 TestReadRows_Retry_LastScannedRow_Reverse
 TestReadRows_Generic_CloseClient
 TestReadRows_Generic_DeadlineExceeded

google-cloud-bigtable/conformance/test_proxy/test_proxy.rb

@@ -223,8 +223,8 @@ def read_rows req, _call
     end
   rescue Google::Cloud::Error => e
     LOGGER.info "ReadRows failed: Caught #{e}"
-    Google::Bigtable::Testproxy::RowResult.new(
-      status: make_status(e.code)
+    Google::Bigtable::Testproxy::RowsResult.new(
+      status: make_status(e.code, e.message)
     )
   end
   # rubocop:enable Metrics/AbcSize

google-cloud-bigtable/lib/google/cloud/bigtable/chunk_processor.rb

@@ -94,12 +94,23 @@ def validate_reset_row
         #   or if family name or column qualifier are empty
         #
         def validate_new_row
-          raise_if row.key, "A new row cannot have existing state"
-          raise_if chunk.row_key.empty?, "A row key must be set"
-          raise_if chunk.reset_row, "A new row cannot be reset"
-          raise_if last_key == chunk.row_key, "A commit happened but the same key followed"
-          raise_if chunk.family_name.nil?, "A family must be set"
-          raise_if chunk.qualifier.nil?, "A column qualifier must be set"
+          validations = [
+            [row.key, "A new row cannot have existing state"],
+            [chunk.row_key.empty?, "A row key must be set"],
+            [chunk.reset_row, "A new row cannot be reset"],
+            [last_key == chunk.row_key, "A commit happened but the same key followed"],
+            [
+              last_key && chunk.row_key < last_key,
+              "Out of order row key, must be strictly increasing. " \
+              "New key: '#{chunk.row_key}', Previous key: '#{last_key}'"
+            ],
+            [chunk.family_name.nil?, "A family must be set"],
+            [chunk.qualifier.nil?, "A column qualifier must be set"]
+          ]
+
+          validations.each do |condition, message|
+            raise_if condition, message
+          end
         end
 
         ##

google-cloud-bigtable/lib/google/cloud/bigtable/rows_reader.rb

@@ -92,6 +92,10 @@ def read rows: nil, filter: nil, rows_limit: nil
               yield row
               @rows_count += 1
             end
+
+            if res.last_scanned_row_key && !res.last_scanned_row_key.empty?
+              @chunk_processor.last_key = res.last_scanned_row_key
+            end
           end
 
           @chunk_processor.validate_last_row_complete

google-cloud-bigtable/test/google/cloud/bigtable/table/read_rows_test.rb

@@ -434,4 +434,109 @@ def mock.read_rows _args
     _(rows.length).must_equal 1
     _(rows.first).must_equal expected_row
   end
+
+  it "reports an error if the client detects that the server sent row keys out of order" do
+    mock = Minitest::Mock.new
+    bigtable.service.mocked_client = mock
+    table = bigtable.table(instance_id, table_id)
+
+    chunks = [
+      Google::Cloud::Bigtable::V2::ReadRowsResponse::CellChunk.new(
+        row_key: "row-2",
+        family_name: Google::Protobuf::StringValue.new(value: "cf"),
+        qualifier: Google::Protobuf::BytesValue.new(value: "cq"),
+        value: "v",
+        commit_row: true
+      ),
+      Google::Cloud::Bigtable::V2::ReadRowsResponse::CellChunk.new(
+        row_key: "row-1", # Out of order
+        family_name: Google::Protobuf::StringValue.new(value: "cf"),
+        qualifier: Google::Protobuf::BytesValue.new(value: "cq"),
+        value: "v",
+        commit_row: true
+      )
+    ]
+    responses = [
+      Google::Cloud::Bigtable::V2::ReadRowsResponse.new(chunks: [chunks[0]]),
+      Google::Cloud::Bigtable::V2::ReadRowsResponse.new(chunks: [chunks[1]])
+    ]
+
+    mock.expect :read_rows, responses,
+                table_name: table_path(instance_id, table_id),
+                rows: Google::Cloud::Bigtable::V2::RowSet.new(row_ranges: [Google::Cloud::Bigtable::V2::RowRange.new]),
+                filter: nil,
+                rows_limit: nil,
+                app_profile_id: nil
+
+    err = assert_raises Google::Cloud::Error do
+      table.read_rows.to_a
+    end
+    _(err.message).must_match /Out of order row key/
+  end
+
+  it "resumes a read from the last scanned row key after a retryable error" do
+    mock = Minitest::Mock.new
+    bigtable.service.mocked_client = mock
+    table = bigtable.table(instance_id, table_id)
+
+    first_mock_responses = [
+      Google::Cloud::Bigtable::V2::ReadRowsResponse.new(last_scanned_row_key: ""),
+      "error"
+    ]
+    first_mock_itr = OpenStruct.new(read_response: first_mock_responses)
+    def first_mock_itr.each
+      self.read_response.each do |res|
+        raise GRPC::DeadlineExceeded, "Deadline exceeded" if res == "error"
+        yield res
+      end
+    end
+
+    second_mock_responses = [
+      Google::Cloud::Bigtable::V2::ReadRowsResponse.new(last_scanned_row_key: "row-a"),
+      "error"
+    ]
+    second_mock_itr = OpenStruct.new(read_response: second_mock_responses)
+    def second_mock_itr.each
+      self.read_response.each do |res|
+        raise GRPC::DeadlineExceeded, "Deadline exceeded" if res == "error"
+        yield res
+      end
+    end
+
+    final_chunk = Google::Cloud::Bigtable::V2::ReadRowsResponse::CellChunk.new(
+      row_key: "row-b",
+      family_name: Google::Protobuf::StringValue.new(value: "cf"),
+      qualifier: Google::Protobuf::BytesValue.new(value: "cq"),
+      value: "v",
+      commit_row: true
+    )
+    final_response = [Google::Cloud::Bigtable::V2::ReadRowsResponse.new(chunks: [final_chunk])]
+
+    mock.expect :read_rows, first_mock_itr,
+                table_name: table_path(instance_id, table_id),
+                rows: Google::Cloud::Bigtable::V2::RowSet.new(row_ranges: [Google::Cloud::Bigtable::V2::RowRange.new]),
+                filter: nil,
+                rows_limit: nil,
+                app_profile_id: nil
+
+    mock.expect :read_rows, second_mock_itr,
+                table_name: table_path(instance_id, table_id),
+                rows: Google::Cloud::Bigtable::V2::RowSet.new(row_ranges: [Google::Cloud::Bigtable::V2::RowRange.new]),
+                filter: nil,
+                rows_limit: nil,
+                app_profile_id: nil
+
+    mock.expect :read_rows, final_response,
+                table_name: table_path(instance_id, table_id),
+                rows: Google::Cloud::Bigtable::V2::RowSet.new(
+                  row_ranges: [Google::Cloud::Bigtable::V2::RowRange.new(start_key_open: "row-a")]
+                ),
+                filter: nil,
+                rows_limit: nil,
+                app_profile_id: nil
+
+    rows = table.read_rows.to_a
+    _(rows.map(&:key)).must_equal ["row-b"]
+    mock.verify
+  end
 end