Bigtable records bulk update (MutateRows RPC call) #3401

CuriousDima · 2017-05-11T17:26:28Z

Implementation of bulk updates for DirectRow (MutateRows RPC call)

garye

Thanks!

It would be good to add a system test or two, which can run against the emulator or the live service.
https://github.com/GoogleCloudPlatform/google-cloud-python/blob/master/bigtable/tests/system.py

bigtable/google/cloud/bigtable/table.py

+
+
+def _mutate_rows_request(table_name, rows):
+    """Creates a request to read rows in a table.


bigtable/google/cloud/bigtable/table.py

+    """Creates a request to read rows in a table.
+
+    :type table_name: str
+    :param table_name: The name of the table to read from.


bigtable/google/cloud/bigtable/table.py

+            mutations_count += 1
+            entry.mutations.add().CopyFrom(mutation)
+    if mutations_count > 100000:
+        raise TooManyMutationsError('Maximum number of the entries mutations '


bigtable/google/cloud/bigtable/table.py

+
+
+def _check_rows(table_name, rows):
+    """Checks that all rows belong to the table.


bigtable/google/cloud/bigtable/table.py

@@ -29,6 +29,14 @@
 from google.cloud.bigtable.row_data import PartialRowsData


+class RowBelongingError(Exception):


bigtable/google/cloud/bigtable/row.py

+
+    @property
+    def table(self):
+        return self._table


bigtable/google/cloud/bigtable/table.py

+
+
+class TooManyMutationsError(Exception):
+    """The number of mutations for bulk request is too big."""


bigtable/google/cloud/bigtable/table.py

+
+        :rtype: list
+        :returns: A list of tuples (``MutateRowsResponse.Entry`` protobuf 
+                  corresponding to the errors, :class:`.DirectRow`) 


bigtable/google/cloud/bigtable/table.py

+        for response in responses:
+            for entry in response.entries:
+                if not entry.status.code:
+                    rows[entry.index].clear()


bigtable/google/cloud/bigtable/table.py

+        responses = client._data_stub.MutateRows(mutate_rows_request)
+        for response in responses:
+            for entry in response.entries:
+                if not entry.status.code:


bigtable/tests/unit/test_row.py

@@ -16,6 +16,26 @@
 import unittest


+class TestRow(unittest.TestCase):


bigtable/tests/unit/test_table.py

+    def test__mutate_rows_too_many_mutations(self):
+        from google.cloud.bigtable.table import TooManyMutationsError
+        from google.cloud.bigtable.row import DirectRow
+        from google.cloud.bigtable._generated.data_pb2 import Mutation


bigtable/tests/unit/test_table.py

+        rows = [DirectRow(row_key=b'row_key', table='table'),
+                DirectRow(row_key=b'row_key_2', table='table')]
+        rows[0]._pb_mutations = [mutation for _ in range(0, 50000)]
+        rows[1]._pb_mutations = [mutation for _ in range(0, 50001)]


bigtable/tests/unit/test_table.py

@@ -569,6 +616,11 @@ def _SampleRowKeysRequestPB(*args, **kw):

    return messages_v2_pb2.SampleRowKeysRequest(*args, **kw)

+def _MutateRowsRequestPB(*args, **kw):


bigtable/tests/unit/test_table.py

@@ -16,6 +16,53 @@
 import unittest


+class Test___mutate_rows_request(unittest.TestCase):


# Conflicts: # bigtable/google/cloud/bigtable/row.py

CuriousDima · 2017-05-21T16:47:28Z

Hey! Thank you for your comments, guys.

I fixed almost all of your comments and leave some comments for other. @garye @dhermes could you please take another look?

CuriousDima · 2017-05-21T17:38:48Z

Also, could you please take a look why cover failed? Strings it points to are covered... Can't figure out.

garye

This is very close from my perspective!

bigtable/google/cloud/bigtable/table.py

+    :rtype: :class:`data_messages_v2_pb2.MutateRowsRequest`
+    :returns: The ``MutateRowsRequest`` protobuf corresponding to the inputs.
+    :raises: :exc:`~.table.TooManyMutationsError` if the number of mutations is
+             grater than 100,000


bigtable/google/cloud/bigtable/table.py

@@ -276,6 +288,34 @@ def read_rows(self, start_key=None, end_key=None, limit=None,
        # We expect an iterator of `data_messages_v2_pb2.ReadRowsResponse`
        return PartialRowsData(response_iterator)

+    def mutate_rows(self, rows):


Each element of the list contains a status of the corresponding row mutations.

CuriousDima · 2017-05-26T11:25:22Z

Fixed. Could you please take another look?

garye

This looks good from my side! We'll just need @dhermes or someone from the python team to approve.

bigtable/google/cloud/bigtable/table.py

@@ -300,21 +300,19 @@ def mutate_rows(self, rows):
        :param rows: List or other iterable of :class:`.DirectRow` instances.

        :rtype: list
-        :returns: A list of tuples (``MutateRowsResponse.Entry`` protobuf
-                  corresponding to the errors, :class:`.DirectRow`)
+        :returns: A list of corresponding to each row statuses.


dhermes · 2017-05-26T16:16:23Z

@garye @skyfox Will do my best to review today.

garye · 2017-05-26T16:21:16Z

Thanks a lot @dhermes. We appreciate all the effort.

dhermes · 2017-05-26T23:50:06Z

Sorry I didn't have a chance today guys. I'll try to take a look this weekend / ASAP Tuesday.

garye · 2017-05-27T00:01:47Z

No worries @dhermes, next week is fine!

dhermes

Why are only DirectRow instances allowed? Is that part of the
method or just part of the implementation?
ISTM that the most "robust" way to actually turn the PB response
into a list of status codes would be to keep a list of tuples
(index, status) and then before returning from the function,
call a helper to coerce that into a list. (You could also use
a dictionary rather than a list of tuples but you want to make
sure you don't set a key that's already in the dictionary.)

bigtable/google/cloud/bigtable/table.py

+            for entry in response.entries:
+                responses_statuses[entry.index] = entry.status
+                if entry.status.code == 0:
+                    rows[entry.index].clear()


bigtable/google/cloud/bigtable/table.py

+            entry.mutations.add().CopyFrom(mutation)
+    if mutations_count > _MAX_BULK_MUTATIONS:
+        raise TooManyMutationsError('Maximum number of mutations is %s' %
+                                    _MAX_BULK_MUTATIONS)


bigtable/google/cloud/bigtable/table.py

+        _check_row_type(row)
+        entry = request_pb.entries.add()
+        entry.row_key = row.row_key
+        for mutation in row._get_mutations(None):


bigtable/google/cloud/bigtable/table.py

+
+
+def _check_row_table_name(table_name, row):
+    """Checks that a row belong to the table.


bigtable/tests/system.py

+        row_2.set_cell(COLUMN_FAMILY_ID1, COL_NAME1, CELL_VAL2)
+        row_2.commit()
+        rows = [row_1, row_2]
+        self.rows_to_delete.extend(rows)


bigtable/tests/unit/test_table.py

+        row_2 = DirectRow(row_key=b'row_key_2', table=table)
+        row_2.set_cell('cf', b'col', b'value2')
+
+        response = MutateRowsResponse()


bigtable/google/cloud/bigtable/table.py

+        client = self._instance._client
+        responses = client._data_stub.MutateRows(mutate_rows_request)
+
+        responses_statuses = [None for _ in range(len(rows))]


bigtable/google/cloud/bigtable/table.py

+                responses_statuses[entry.index] = entry.status
+                if entry.status.code == 0:
+                    rows[entry.index].clear()
+        return responses_statuses


bigtable/google/cloud/bigtable/table.py

+        responses_statuses = [None for _ in range(len(rows))]
+        for response in responses:
+            for entry in response.entries:
+                responses_statuses[entry.index] = entry.status


bigtable/google/cloud/bigtable/table.py

+        :param rows: List or other iterable of :class:`.DirectRow` instances.
+
+        :rtype: list
+        :returns: A list of corresponding to each row statuses.


bigtable/google/cloud/bigtable/table.py

+    """
+    if not isinstance(row, DirectRow):
+        raise TypeError('Bulk processing can not be applied for '
+                        'conditional or append mutations.')


bigtable/tests/unit/test_table.py

+        bigtable_pb2 as data_messages_v2_pb2)
+
+    return data_messages_v2_pb2.MutateRowsRequest(*args, **kw)
+


bigtable/google/cloud/bigtable/table.py

@@ -29,6 +29,18 @@
 from google.cloud.bigtable.row_data import PartialRowsData


+# Maximum number of mutations in bulk
+_MAX_BULK_MUTATIONS = 100000


CuriousDima · 2017-06-09T15:25:01Z

Hey! Could you please take another look?

@dhermes asked: "Why are only DirectRow instances allowed? Is that part of the method or just part of the implementation?"
Only DirectRows (primitive mutations-based) can be processed in bulk.

googlebot · 2017-06-09T16:35:20Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

garye

@dhermes @tseaver This seems ok from my side, can you take another look?

Thanks!

Dmitry Timofeev added 3 commits April 27, 2017 17:38

Update rows in bulk: initial version

2a245e6

Update rows in bulk

e194f7c

Merge remote-tracking branch 'upstream/master'

60d7f36

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 11, 2017

CuriousDima changed the title ~~Records bulk update (MutateRows RPC call)~~ Bigtable records bulk update (MutateRows RPC call) May 11, 2017

CuriousDima mentioned this pull request May 11, 2017

Implement multi-row mutations for python Bigtable #2411

Closed

garye suggested changes May 11, 2017

View reviewed changes

dhermes reviewed May 11, 2017

View reviewed changes

BT bulk update: first set of comments' fixes

6fa7796

CuriousDima mentioned this pull request May 12, 2017

Getters for row key and row table #3408

Merged

Dmitry Timofeev and others added 3 commits May 16, 2017 10:37

Resolved merge conflict.

c986d39

Merge branch 'master' into master

56aa01d

Merge remote-tracking branch 'origin/master'

f8f030f

# Conflicts: # bigtable/google/cloud/bigtable/row.py

tseaver added api: bigtable Issues related to the Bigtable API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels May 17, 2017

Dmitry Timofeev added 3 commits May 21, 2017 13:10

Fix the second part of comments.

2382f03

BT bulk update: system test for mutate_rows()

edd7d62

Merge upstream/master

738c700

Dmitry Timofeev added 2 commits May 21, 2017 18:19

Fix some minor lint warnings.

871cc16

Fix some minor lint warnings.

a2e9196

garye suggested changes May 23, 2017

View reviewed changes

Return a list that is the same size as the input rows.

8cbf1cd

Each element of the list contains a status of the corresponding row mutations.

garye approved these changes May 26, 2017

View reviewed changes

dhermes reviewed May 31, 2017

View reviewed changes

tseaver suggested changes Jun 5, 2017

View reviewed changes

Dmitry Timofeev added 3 commits June 9, 2017 16:00

Fix tests and some minor code comments

65ad0e4

Fix lint warnings (string too long)

d70ef85

Link to the BT docs with limit specification.

b31a08b

Add anchor to the method on the RPC docs page.

ce995cf

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 9, 2017

garye approved these changes Jun 16, 2017

View reviewed changes

tseaver approved these changes Jun 19, 2017

View reviewed changes

tseaver merged commit 6618165 into googleapis:master Jun 19, 2017

tseaver mentioned this pull request Jun 26, 2017

Prep bigtable-0.25.0 release. #3534

Merged

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017

Allow bulk update of records via 'MutateRows' API (googleapis#3401)

8af036a

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017

Allow bulk update of records via 'MutateRows' API (googleapis#3401)

0eb79fa

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017

Allow bulk update of records via 'MutateRows' API (googleapis#3401)

c4b91d4



		def _mutate_rows_request(table_name, rows):
		"""Creates a request to read rows in a table.



		def _check_rows(table_name, rows):
		"""Checks that all rows belong to the table.

		@@ -29,6 +29,14 @@
		from google.cloud.bigtable.row_data import PartialRowsData


		class RowBelongingError(Exception):



		class TooManyMutationsError(Exception):
		"""The number of mutations for bulk request is too big."""

		@@ -16,6 +16,26 @@
		import unittest


		class TestRow(unittest.TestCase):

		@@ -569,6 +616,11 @@ def _SampleRowKeysRequestPB(args, *kw):

		return messages_v2_pb2.SampleRowKeysRequest(args, *kw)

		def _MutateRowsRequestPB(args, *kw):

		@@ -16,6 +16,53 @@
		import unittest


		class Test___mutate_rows_request(unittest.TestCase):



		def _check_row_table_name(table_name, row):
		"""Checks that a row belong to the table.

		bigtable_pb2 as data_messages_v2_pb2)

		return data_messages_v2_pb2.MutateRowsRequest(args, *kw)

Bigtable records bulk update (MutateRows RPC call) #3401

Bigtable records bulk update (MutateRows RPC call) #3401

Conversation

CuriousDima commented May 11, 2017

garye 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.

CuriousDima commented May 21, 2017

CuriousDima commented May 21, 2017

garye 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.

CuriousDima commented May 26, 2017

garye left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

dhermes commented May 26, 2017

garye commented May 26, 2017

dhermes commented May 26, 2017

garye commented May 27, 2017

dhermes 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.

CuriousDima commented Jun 9, 2017

googlebot commented Jun 9, 2017

garye left a comment

Choose a reason for hiding this comment