-
Notifications
You must be signed in to change notification settings - Fork 36
feat: Adding support for exclude_txn_from_change_streams option #368
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.
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
feat: Adding support for exclude_txn_from_change_streams option #368
Conversation
olavloite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a few minor nits
lib/spanner_client_ext.rb
Outdated
|
|
||
| class Session | ||
| def commit_transaction transaction, mutations = [], commit_options: nil | ||
| def commit_transaction transaction, mutations = [], commit_options: nil, exclude_txn_from_change_streams: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think we need this in the commit request. This only goes in the BeginTransaction request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took reference from Ruby client library where this attribute was getting set during the commit call. For ref:https://github.com/googleapis/ruby-spanner/blob/a2e5c8c8b8a909746bbc9aff1b79884ed44db104/google-cloud-spanner/lib/google/cloud/spanner/session.rb#L656
But anyways i will remove this from this commit as it is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I understand that. The difference here is that:
- A CommitRequest can also start a transaction. This is called a single-use write-only transaction, and in such a case the CommitRequest both starts and commits the transaction. This is sometimes used for bulk writing. The drawback of these transactions is that there is no guarantee that they will be applied only once. It could be that a component somewhere in the network path between the client and the server replays the request, for example due to a temporary network problem leading to an UNAVAILABLE error, and in that case the transaction would be applied twice.
- Ruby ActiveRecord always starts the transaction first, meaning that the CommitRequest will never start a transaction. So in this case we never need this option here.
| assert_equal 1000, commit_options[:max_commit_delay] | ||
| end | ||
|
|
||
| def test_exclude_txn_from_change_streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test does not really appear to test anything relevant. The exclude_txn_from_change_streams flag goes into the BeginTransaction options, but this test sets that flag after the transaction has started, and dos not verify anything regarding the BeginTransaction options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. At first, I believed this attribute had to be set during the commit call. I've now updated the test to assert the TransactionSelector object assigned during the transaction.begin call instead.
|
|
||
| def test_upsert_all_dml_with_exclude_from_change_streams | ||
| sql = "INSERT OR UPDATE INTO `singers` (`id`,`first_name`,`last_name`) " + | ||
| "VALUES (1, 'Dave', 'Allison'), (2, 'Alice', 'Davidson'), (3, 'Rene', 'Henderson')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This SQL statement would in the real world not return 3 rows, it would only return an update count. It would slightly improve the readability of this test to add THEN RETURN * to the SQL string, so it matches what the mock return value is.
…ption is set correctly
feat: Adding support for
exclude_txn_from_change_streamsoption while creating RW transactions.This PR introduces support for the
exclude_txn_from_change_streams optionfor RW transactions. This allows developers to selectively prevent specific transactions from being recorded in a database's change stream.Usage Example:
Fixes #367