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.

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!: remove session pool preparing #515

Merged
merged 4 commits into from Oct 21, 2020
Merged

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 15, 2020

Removes the feature for preparing sessions in the session pool. This is step 2 of inlining BeginTransaction with the first statement of a transaction.

@olavloite olavloite requested a review from as a code owner Oct 15, 2020
@google-cla google-cla bot added the cla: yes label Oct 15, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 15, 2020

Codecov Report

Merging #515 into inline-begin-tx will decrease coverage by 0.18%.
The diff coverage is 94.52%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##             inline-begin-tx     #515      +/-   ##
=====================================================
- Coverage              82.31%   82.12%   -0.19%     
- Complexity              2419     2429      +10     
=====================================================
  Files                    138      138              
  Lines                  13518    13480      -38     
  Branches                1269     1299      +30     
=====================================================
- Hits                   11127    11071      -56     
+ Misses                  1885     1881       -4     
- Partials                 506      528      +22     
Impacted Files Coverage Δ Complexity Δ
.../google/cloud/spanner/MetricRegistryConstants.java 95.23% <ø> (ø) 1.00 <0.00> (ø)
.../java/com/google/cloud/spanner/SpannerOptions.java 89.33% <ø> (-0.15%) 39.00 <0.00> (-1.00)
...ain/java/com/google/cloud/spanner/SessionPool.java 85.87% <90.69%> (-0.36%) 70.00 <0.00> (-39.00)
...gle/cloud/spanner/AsyncTransactionManagerImpl.java 69.23% <100.00%> (-3.50%) 10.00 <0.00> (-2.00)
...a/com/google/cloud/spanner/DatabaseClientImpl.java 84.15% <100.00%> (+9.15%) 19.00 <4.00> (-15.00) ⬆️
...ain/java/com/google/cloud/spanner/SessionImpl.java 86.06% <100.00%> (-0.93%) 29.00 <2.00> (-1.00)
...a/com/google/cloud/spanner/SessionPoolOptions.java 89.65% <100.00%> (-0.12%) 15.00 <0.00> (-1.00)
...ain/java/com/google/cloud/spanner/SpannerImpl.java 93.80% <100.00%> (-0.06%) 28.00 <1.00> (ø)
...m/google/cloud/spanner/TransactionManagerImpl.java 88.67% <100.00%> (-0.21%) 20.00 <1.00> (-2.00)
...om/google/cloud/spanner/TransactionRunnerImpl.java 83.96% <100.00%> (-1.45%) 9.00 <0.00> (ø)
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07346f0...c547f86. Read the comment docs.

@olavloite olavloite requested review from skuruppu and thiagotnunes Oct 15, 2020
@product-auto-label product-auto-label bot added the api: spanner label Oct 16, 2020
PooledSessionFuture session =
mode == SessionMode.READ_WRITE ? getReadWriteSession() : getReadSession();
private <T> T runWithSessionRetry(Function<Session, T> callable) {
PooledSessionFuture session = getReadSession();
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 18, 2020

Choose a reason for hiding this comment

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

nit: could we rename methods such as getReadSession() to getSession() now that we do not have such separation?

Copy link
Contributor Author

@olavloite olavloite Oct 20, 2020

Choose a reason for hiding this comment

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

Done.

@@ -36,9 +36,14 @@
private static final LabelValue UNSET_LABEL = LabelValue.create(null);

static final LabelValue NUM_IN_USE_SESSIONS = LabelValue.create("num_in_use_sessions");

@Deprecated
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 18, 2020

Choose a reason for hiding this comment

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

nit: could we add javadocs on the deprecation notice?

Copy link
Contributor Author

@olavloite olavloite Oct 20, 2020

Choose a reason for hiding this comment

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

Done.

} else {
span.addAnnotation("Acquired read only session");
span.addAnnotation("Acquired rsession");
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 18, 2020

Choose a reason for hiding this comment

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

nit: typo rsession -> session

Copy link
Contributor Author

@olavloite olavloite Oct 20, 2020

Choose a reason for hiding this comment

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

Done.

@@ -765,7 +762,7 @@ public void setSpan(Span span) {
new Callable<T>() {
@Override
public T call() {
boolean useInlinedBegin = inlineBegin;
boolean useInlinedBegin = true;
if (attempt.get() > 0) {
if (useInlinedBegin) {
Copy link
Contributor

@thiagotnunes thiagotnunes Oct 18, 2020

Choose a reason for hiding this comment

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

Could we remove the branching here, since useInlinedBegin is always true?

Copy link
Contributor Author

@olavloite olavloite Oct 20, 2020

Choose a reason for hiding this comment

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

Yep, that's quite useless. Thanks for noticing.

@thiagotnunes thiagotnunes merged commit 2768f69 into inline-begin-tx Oct 21, 2020
15 of 16 checks passed
@thiagotnunes thiagotnunes deleted the remove-session-prepare branch Oct 21, 2020
thiagotnunes pushed a commit that referenced this issue Oct 23, 2020
* feat: inline begin tx with first statement

* feat: support inlining BeginTransaction

* fix: invalid dml statement can still return tx id

* bench: add benchmarks for inline begin

* feat: add inline begin for async runner

* test: add additional tests and ITs

* test: add tests for error during tx

* test: use statement with same error code on emulator

* test: skip test on emulator

* test: constraint error causes transaction to be invalidated

* fix: retry transaction if first statements fails and had BeginTransaction option

* fix: handle aborted exceptions

* test: add additional tests for corner cases

* feat: use single-use tx for idem-potent mutations

* fix: remove check for idempotent mutations

* chore: remove commented code

* feat!: remove session pool preparing (#515)

* feat: remove session pool preparing

* fix: fix integration tests

* test: fix malformed retry loop in test case

* fix: review comments

* chore: run formatter

* test: fix integration test that relied on data from other test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants