Fix Failing BigTable System tests

[theacodes]

https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/6646

[crwilcox]

I think this is related to the merge #5302 as it was a pretty large scale modification.

[crwilcox]

Currently the system tests for BigTable are failing. It appears this is a result of the recent rewrite/regen of the library.

@aneepct, do you want to take a look at these? The tests failing are all in /bigtable/tests/system.py and currently marked with xfail

[tseaver]

At least some of the failures are due to the fact that since 86102c9, Client.list_instances now returns protos, rather than google.cloud.bigtable.instance.Instance objects (and failed locations).

[tseaver]

xfail'ed system tests:

• test_list_instances (fixed, un-xfail'ed in PR Make 'Client.list_instances' return actual instance objects, not protos. #5420)
• the rest (fixed by BigTable: Fixing client for system tests #5398, un-xfail'ed in PR Unmark most of the xfailed tests. #5421)
• test_read_large_cell_limit blows up due to (StatusCode.RESOURCE_EXHAUSTED, Received message larger than max (10485818 vs. 4194304). This looks like a real bug (we used to be able to send the large value).

@aneepct, @sduskis Is there a reason why we can no longer send the 10Mb cell value?

[sduskis]

The 10MB issue is a grpc setting, IIUC. It's not about the CBT related code directly.

Python bug related to this
Python stack overflow discussion
Node.js bug related to this

[tseaver]

@sduskis FWIW, the documented limit for cell values is ~10 MB per cell.

[sduskis]

@tseaver: The documented limit only affects the CBT service. I believe that the problem at hand is a client-side grpc setting, and client-side validation.

[tseaver]

@sduskis How would the back-end folks expect users to set cell values larger than an (undocumented) 4 Mb limit, then? AFAIK, there is no "streaming PUT" notion in the API.

[sduskis]

@tseaver: All clients need the magical grpc setting (MAX_MESSAGE_SIZE) that would allow the grpc client to send values larger than 4MB. This is something that the client needs to do, and not something that end users have to fix.

[sduskis]

@tseaver I think that we did something about this in the python client in the past, as per #2880. A setting was probably reverted, or something like that, that caused this breakage.

[crwilcox]

I looked at https://github.com/GoogleCloudPlatform/google-cloud-python/pull/2907/files, the PR that addressed #2880 and it calls out # NOTE: 'grpc.max_message_length' will no longer be recognized in grpcio 1.1 and later.. So I think it is possible it wasn't so much reverted as no longer possible with the updated versions of grpcio.

[tseaver]

@crwilcox I believe grpc.max_message_length has been superseded by grpc.max_send_message_length and grpc.max_receive_message_length. Those two values are set in the pubsub clients channel setups: publisher and subscriber).

ISTM that the bigtable_v2.gapic.bigtable_client.BigtableClient should be setting those values appropriately for its channel. Because that is generated code, I'm assuming that there is some config lying around which is used to drive that.

[tseaver]

@theacodes, @crwilcox I can confirm that the following patch gets the last xfailed Bigtable system test to pass:

diff --git a/bigtable/google/cloud/bigtable_v2/gapic/bigtable_client.py b/bigtable/google/cloud/bigtable_v2/gapic/bigtable_client.py
index 60eb063..5172fb5 100644
--- a/bigtable/google/cloud/bigtable_v2/gapic/bigtable_client.py
+++ b/bigtable/google/cloud/bigtable_v2/gapic/bigtable_client.py
@@ -97,6 +97,10 @@ class BigtableClient(object):
                 self.SERVICE_ADDRESS,
                 credentials=credentials,
                 scopes=self._DEFAULT_SCOPES,
+                options={
+                    'grpc.max_send_message_length': -1,
+                    'grpc.max_receive_message_length': -1,
+                }.items(),
             )
 
         # Create the gRPC stubs.
diff --git a/bigtable/tests/system.py b/bigtable/tests/system.py
index 2a1bbdc..43486d5 100644
--- a/bigtable/tests/system.py
+++ b/bigtable/tests/system.py
@@ -408,8 +408,6 @@ class TestDataAPI(unittest.TestCase):
         self.assertEqual(
             row2_data.cells[COLUMN_FAMILY_ID1][COL_NAME1][0].value, CELL_VAL4)
 
-    @pytest.mark.xfail(reason="https://github.com/GoogleCloudPlatform/"
-                              "google-cloud-python/issues/5362")
     def test_read_large_cell_limit(self):
         row = self._table.row(ROW_KEY)
         self.rows_to_delete.append(row)

[tseaver]

#5360 added new (non-flaky) system test failures under Python 3. I fixed them this morning in #5474.

[tseaver]

New system test failure in test_list_instances:

___________________ TestInstanceAdminAPI.test_list_instances ___________________

self = <tests.system.TestInstanceAdminAPI testMethod=test_list_instances>

    def test_list_instances(self):
        instances, failed_locations = Config.CLIENT.list_instances()
        self.assertEqual(failed_locations, [])
        # We have added one new instance in `setUpModule`.
>       self.assertEqual(len(instances), len(EXISTING_INSTANCES) + 1)
E       AssertionError: 3 != 1

This one is flaky: I can't get it to fail on my machine. It is possible that the extra instances are due to another system test run.

[tseaver]

#5476 should close the new failure I reported earlier today.

[tseaver]

@theacodes, @crwilcox Should I go ahead and create a PR which manually patches in the max_{send,receive}_message_length bits, and removes the xfail for the large cell test? I'm not sure where / how to request that the codegen for Bigtable gets updated to include those overrides.

[theacodes]

go for it. We don't have means to override this in the codegen.

…

On Thu, Jun 14, 2018 at 12:11 PM Tres Seaver ***@***.***> wrote: @theacodes <https://github.com/theacodes>, @crwilcox <https://github.com/crwilcox> Should I go ahead and create a PR which manually patches in the max_{send,receive}_message_length bits, and removes the xfail for the large cell test? I'm not sure where / how to request that the codegen for Bigtable gets updated to include those overrides. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5362 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPUczPvumIS6x91eXnOxHi3MTnmllQnks5t8rVNgaJpZM4UJJ-w> .