Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add maxBatchingDelayMs Option. #987

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Add maxBatchingDelayMs Option. #987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2623409
Modify batch.py
nginsberg-google Jul 31, 2023
b65fbd1
Modify transaction.py
nginsberg-google Jul 31, 2023
7d2de08
Merge branch 'main' into batching-delay
parthea Aug 2, 2023
f12f13c
Merge branch 'main' into batching-delay
surbhigarg92 Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion google/cloud/spanner_v1/batch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from google.cloud.spanner_v1._helpers import _retry
from google.cloud.spanner_v1._helpers import _check_rst_stream_error
from google.api_core.exceptions import InternalServerError
from google.protobuf.duration_pb2 import Duration


class _BatchBase(_SessionWrapper):
Expand Down Expand Up @@ -145,9 +146,14 @@ def _check_state(self):
if self.committed is not None:
raise ValueError("Batch already committed")

def commit(self, return_commit_stats=False, request_options=None):
def commit(self, max_batching_delay_ms=None, return_commit_stats=False, request_options=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding new argument max_batching_delay_ms as second parameter might break any existing applications that have not used key explicitly while calling commit.

Can we move this to end?

Suggested change
def commit(self, max_batching_delay_ms=None, return_commit_stats=False, request_options=None):
def commit(self, return_commit_stats=False, request_options=None, max_batching_delay_ms=None):

@surbhigarg92 Can you please correct me this is incorrect?

"""Commit mutations to the database.

:type max_batching_delay_ms: Optional[int]
:param max_batching_delay_ms:
If present, the The amount of latency this request is willing to incur in order to
improve throughput.

:type return_commit_stats: bool
:param return_commit_stats:
If true, the response will return commit stats which can be accessed though commit_stats.
Expand Down Expand Up @@ -182,11 +188,16 @@ def commit(self, return_commit_stats=False, request_options=None):
# Request tags are not supported for commit requests.
request_options.request_tag = None

Duration max_batching_delay = None
if max_batching_delay_ms is not None:
max_batching_delay.nanos = 1000000 * [max_batching_delay_ms]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this default decided?


request = CommitRequest(
session=self._session.name,
mutations=self._mutations,
single_use_transaction=txn_options,
return_commit_stats=return_commit_stats,
max_batching_delay=max_batching_delay,
request_options=request_options,
)
with trace_call("CloudSpanner.Commit", self._session, trace_attributes):
Expand Down
13 changes: 12 additions & 1 deletion google/cloud/spanner_v1/transaction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from google.cloud.spanner_v1 import RequestOptions
from google.api_core import gapic_v1
from google.api_core.exceptions import InternalServerError
from google.protobuf.duration_pb2 import Duration


class Transaction(_SnapshotBase, _BatchBase):
Expand Down Expand Up @@ -180,9 +181,14 @@ def rollback(self):
self.rolled_back = True
del self._session._transaction

def commit(self, return_commit_stats=False, request_options=None):
def commit(self, max_batching_delay_ms=None, return_commit_stats=False, request_options=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above comment

"""Commit mutations to the database.

:type max_batching_delay_ms: Optional[int]
:param max_batching_delay_ms:
If present, the The amount of latency this request is willing to incur in order to
improve throughput.

:type return_commit_stats: bool
:param return_commit_stats:
If true, the response will return commit stats which can be accessed though commit_stats.
Expand Down Expand Up @@ -223,11 +229,16 @@ def commit(self, return_commit_stats=False, request_options=None):
# Request tags are not supported for commit requests.
request_options.request_tag = None

Duration max_batching_delay = None
if max_batching_delay_ms is not None:
max_batching_delay.nanos = 1000000 * [max_batching_delay_ms]

request = CommitRequest(
session=self._session.name,
mutations=self._mutations,
transaction_id=self._transaction_id,
return_commit_stats=return_commit_stats,
max_batching_delay=max_batching_delay,
request_options=request_options,
)
with trace_call("CloudSpanner.Commit", self._session, trace_attributes):
Expand Down