feat: support commit_options #364

aakashanandg · 2025-06-05T15:39:15Z

feat: Add support for commit_options with max_commit_delay and return_commit_stats for transaction commits.

This change introduces the ability to configure transaction commit behavior using the commit_options parameter. Specifically, it enables setting:

max_commit_delay: To fine-tune commit behavior for throughput-optimized writes.
return_commit_stats: To request commit statistics.

Key changes:

Modified the service.commit method to accept a commit_options parameter.
Implemented logic to handle both max_commit_delay and return_commit_stats within the commit_options parameter.
Added unit and mock server tests to test out the functionality.

olavloite

Would you mind also adding a mock server test to verify that the commit options are actually being converted to the correct proto options and included with the commit request?

olavloite · 2025-06-05T19:01:27Z

lib/activerecord_spanner_adapter/transaction.rb

+      @return_commit_stats = options.fetch :return_commit_stats, false
+      @max_commit_delay = options.fetch :max_commit_delay, 0


nit: would it maybe make sense to just keep these together in one field? So just simply @commit_options=options

Yes. I think it make sense to introduce a commit_options hash here and use that instead. I have updated my implementation to use these changes.

olavloite · 2025-06-05T19:02:17Z

lib/activerecord_spanner_adapter/transaction.rb

+    def set_commit_options options = {}
+      return if options.empty?
+
+      options.each do |key, value|


See also my comment above: would it maybe simplify things if we just keep all commit options in one hash, instead of extracting them to separate fields here?

Yes i agree. We can use a single hash and that will be more simpler to use. I have addressed this in the updated commits.

aakashanandg · 2025-06-06T11:24:00Z

Would you mind also adding a mock server test to verify that the commit options are actually being converted to the correct proto options and included with the commit request?

I have added a mock server test with the new commit.

… commit_options

olavloite · 2025-06-06T12:23:25Z

test/activerecord_spanner_mock_server/inline_begin_transaction_test.rb

+        assert_equal _transaction_isolation_level_to_grpc(isolation),
+                    sql_request.transaction&.begin&.isolation_level
+
+        @mock.requests.clear


nit: you can remove this. The requests are automatically cleared after each test

It looks like it is still there :-)

I guess its removed in the recent commit. Can you look at the changes from all commits that shows that it was removed.

olavloite · 2025-06-06T12:26:01Z

lib/activerecord_spanner_adapter/transaction.rb

+    # Sets the commit options for this transaction.
+    # This is used to set the options for the commit RPC, such as return_commit_stats and max_commit_delay.
+    def set_commit_options commit_options = {}
+      @commit_options.merge! commit_options


I think that this might be a bit confusing. The name is set_commit_options, but the implementation is merge. That means that if you call this method twice, then the result will be the combination of the two. The name set_commit_options however sounds like something that will overwrite the previous setting.

Note: I'm necessarily against the merge behavior, but it should be in line with the name of the method.

The merge! operation works on the current state of @commit_options, preserving previous changes and only overwriting the keys that are passed in the new call.

My idea was to maintain a state of previous values, but i think this setter should reset the state entirely with the new state even if some attributes are missing.

Something like this
def set_commit_options options # rubocop:disable Naming/AccessorMethodName return if options.nil? @commit_options = options.dup end

olavloite · 2025-06-06T12:27:52Z

lib/activerecord_spanner_adapter/transaction.rb

        force_begin_read_write if @committable && !@mutations.empty? && !@grpc_transaction
-
-        @connection.session.commit_transaction @grpc_transaction, @mutations if @committable && @grpc_transaction
+        com_options = commit_options


nit: you don't need a separate variable here, you can just use commit_options directly in the call to commit_transaction below.

olavloite · 2025-06-06T12:45:40Z

lib/active_record/connection_adapters/spanner/database_statements.rb

        # Transaction

-        def transaction requires_new: nil, isolation: nil, joinable: true
+        def transaction requires_new: nil, isolation: nil, commit_options: nil, joinable: true


You can simplify this method to the following:

def transaction requires_new: nil, isolation: nil, joinable: true, **kwargs commit_options = kwargs.delete :commit_options if !requires_new && current_transaction.joinable? return super end backoff = 0.2 begin super do # Once the transaction has been started by `super`, apply your custom options # to the Spanner transaction object. if commit_options && @connection.current_transaction @connection.current_transaction.set_commit_options commit_options end # Now, yield to the user's code block. yield end rescue ActiveRecord::StatementInvalid => err if err.cause.is_a? Google::Cloud::AbortedError sleep(delay_from_aborted(err) || (backoff *= 1.3)) retry end raise end end

olavloite · 2025-06-06T12:47:35Z

lib/activerecord_spanner_adapter/transaction.rb

+    attr_reader :commit_options

-    def initialize connection, isolation
+    DEFAULT_COMMIT_OPTIONS = {


nit: I think it would be better if the default is just nil, and that we only include commit options with the actual commit if anything has been set.

Makes sense will update it.

Right now i have not added any sort of validations on the commit_options fields. Should we add more validation on this before making an API request or rely on spanner backend to handle such validations and report error if something is wrong.

Just let Spanner handle any invalid values.

olavloite · 2025-06-06T12:47:53Z

lib/activerecord_spanner_adapter/transaction.rb

+    }.freeze
+
+
+    def initialize connection, isolation, commit_options = DEFAULT_COMMIT_OPTIONS


so this should then preferably also be:

Suggested change

def initialize connection, isolation, commit_options = DEFAULT_COMMIT_OPTIONS

def initialize connection, isolation, commit_options = nil

olavloite · 2025-06-06T14:46:20Z

lib/activerecord_spanner_adapter/transaction.rb

+    # Sets the commit options for this transaction.
+    # This is used to set the options for the commit RPC, such as return_commit_stats and max_commit_delay.
+    def set_commit_options options # rubocop:disable Naming/AccessorMethodName
+      return if options.nil?


nit: this should also set @options to nil

so something like this:

if options.nil? @options = nil else @options = options.dup end

olavloite · 2025-06-06T14:46:47Z

test/activerecord_spanner_mock_server/inline_begin_transaction_test.rb

+        assert_equal _transaction_isolation_level_to_grpc(isolation),
+                    sql_request.transaction&.begin&.isolation_level
+
+        @mock.requests.clear


It looks like it is still there :-)

aakashanandg added 3 commits June 5, 2025 15:21

feat: Adding support for max_commit_delay commit_optins

be3427d

feat: Adding support for commit_options while commiting transactions

f5bdb70

feat: refactored set_commit_options method

fb3e408

aakashanandg requested review from a team and olavloite as code owners June 5, 2025 15:39

product-auto-label bot added the api: spanner Issues related to the googleapis/ruby-spanner-activerecord API. label Jun 5, 2025

aakashanandg changed the title ~~Max commit delay~~ Add commit_options for transaction commits. Jun 5, 2025

olavloite reviewed Jun 5, 2025

View reviewed changes

aakashanandg force-pushed the max-commit-delay branch 2 times, most recently from c27ab31 to fc5578f Compare June 6, 2025 12:02

test: Added mock server tests for testing the CommitRequest proto has…

c65e55e

… commit_options

aakashanandg force-pushed the max-commit-delay branch from fc5578f to c65e55e Compare June 6, 2025 12:04

olavloite reviewed Jun 6, 2025

View reviewed changes

aakashanandg changed the title ~~Add commit_options for transaction commits.~~ feat: support commit_options Jun 6, 2025

aakashanandg force-pushed the max-commit-delay branch 3 times, most recently from 57134e0 to b32560c Compare June 6, 2025 14:40

olavloite previously approved these changes Jun 6, 2025

View reviewed changes

aakashanandg dismissed olavloite’s stale review via 0d10f2d June 6, 2025 15:36

aakashanandg force-pushed the max-commit-delay branch from b32560c to 0d10f2d Compare June 6, 2025 15:36

test: fixed the mock server test to use one isolation type

f0a9f31

aakashanandg force-pushed the max-commit-delay branch from 0d10f2d to f0a9f31 Compare June 6, 2025 15:41

olavloite approved these changes Jun 6, 2025

View reviewed changes

aakashanandg merged commit 0a1020a into googleapis:main Jun 6, 2025
41 checks passed

release-please bot mentioned this pull request Jun 6, 2025

chore(main): release activerecord-spanner-adapter 2.4.0 #366

Open

		@return_commit_stats = options.fetch :return_commit_stats, false
		@max_commit_delay = options.fetch :max_commit_delay, 0

		}.freeze


		def initialize connection, isolation, commit_options = DEFAULT_COMMIT_OPTIONS

feat: support commit_options #364

feat: support commit_options #364

Uh oh!

Conversation

aakashanandg commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aakashanandg commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aakashanandg Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aakashanandg Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aakashanandg commented Jun 5, 2025 •

edited

Loading

aakashanandg Jun 6, 2025 •

edited

Loading

aakashanandg Jun 6, 2025 •

edited

Loading