fix(spanner): ExcludeTransactionFromChangeStreamsOption not always propagated #15372
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15372 +/- ##
=======================================
Coverage 93.02% 93.03%
=======================================
Files 2403 2403
Lines 219441 219466 +25
=======================================
+ Hits 204144 204172 +28
+ Misses 15297 15294 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (current.has<spanner::ExcludeTransactionFromChangeStreamsOption>() && | ||
| current.get<spanner::ExcludeTransactionFromChangeStreamsOption>()) { | ||
| begin.mutable_options()->set_exclude_txn_from_change_streams(true); | ||
| } |
There was a problem hiding this comment.
With the knowledge that bool() == false, save yourself a second lookup by removing the has().
That said, this should have been taken care of by line 571, so methinks there is actually some call to transaction.cc:MakeOpts() missing, or falling short of the mark, when creating options.
There was a problem hiding this comment.
Reworked this change to use CommitOptions like the existing settings are using.
There was a problem hiding this comment.
This doesn't look right.
spanner::CommitOptionsis a deprecated class, replaced byOptions, so it shouldn't be extended. (Indeed, it should be removed/hidden in 3.0.)- The new
ConnectionImplTest.CommitSuccessExcludeFromChangeStreamsExplicitTxntest passes without any of these changes. I can't see b/346858290 so I can't even guess what the issue might be.
There was a problem hiding this comment.
I created a draft PR demonstrating the issue #15391
scotthart
left a comment
scotthart
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @devbww)
| if (current.has<spanner::ExcludeTransactionFromChangeStreamsOption>() && | ||
| current.get<spanner::ExcludeTransactionFromChangeStreamsOption>()) { | ||
| begin.mutable_options()->set_exclude_txn_from_change_streams(true); | ||
| } |
There was a problem hiding this comment.
Reworked this change to use CommitOptions like the existing settings are using.
| // Introduce additional scope here to ensure that when txn is destroyed | ||
| // the session_pool contained by the Connection is still present, such that, | ||
| // the session associated with the transaction can be returned to the pool. |
There was a problem hiding this comment.
Introducing a new scope here does not appear to change the lifetime of any object. It is at the end of its enclosing scope after all.
There was a problem hiding this comment.
As usual, you're correct. The added scope hid the fact that it was the order of destruction that mattered.
scotthart
force-pushed
the
spanner_fix_exclude_txn
branch
from
a86712c to
5bd986a
Compare
scotthart
left a comment
scotthart
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @devbww)
| if (current.has<spanner::ExcludeTransactionFromChangeStreamsOption>() && | ||
| current.get<spanner::ExcludeTransactionFromChangeStreamsOption>()) { | ||
| begin.mutable_options()->set_exclude_txn_from_change_streams(true); | ||
| } |
There was a problem hiding this comment.
I created a draft PR demonstrating the issue #15391
| // Introduce additional scope here to ensure that when txn is destroyed | ||
| // the session_pool contained by the Connection is still present, such that, | ||
| // the session associated with the transaction can be returned to the pool. |
There was a problem hiding this comment.
As usual, you're correct. The added scope hid the fact that it was the order of destruction that mattered.
scotthart
force-pushed
the
spanner_fix_exclude_txn
branch
from
5bd986a to
1812870
Compare
3 participants
This change is