-
Notifications
You must be signed in to change notification settings - Fork 113
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: move session lastUseTime parameter from PooledSession to SessionImpl class. Fix updation of the parameter for chained RPCs within one transaction. #2704
Changes from 21 commits
edc5bbf
49a85df
4cd497b
4a6aa8e
b2aa09d
8d6d71e
77e6e7d
e8b7fad
8aa84e1
922f324
b544080
9862265
5e5f769
a385ceb
fd3bb41
4864053
f5b82fa
73f0192
ff32178
1316579
80dd971
1acd645
6af8187
999a39b
ec80d6a
6cdef81
593a10b
ced1e06
0485aee
c4163d8
b75b19f
86327e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,8 @@ abstract static class Builder<B extends Builder<?, T>, T extends AbstractReadCon | |
private QueryOptions defaultQueryOptions = SpannerOptions.Builder.DEFAULT_QUERY_OPTIONS; | ||
private ExecutorProvider executorProvider; | ||
|
||
private Clock clock; | ||
|
||
Builder() {} | ||
|
||
@SuppressWarnings("unchecked") | ||
|
@@ -110,6 +112,11 @@ B setExecutorProvider(ExecutorProvider executorProvider) { | |
return self(); | ||
} | ||
|
||
B setClock(Clock clock) { | ||
this.clock = clock; | ||
return self(); | ||
} | ||
|
||
abstract T build(); | ||
} | ||
|
||
|
@@ -392,6 +399,8 @@ void initTransaction() { | |
private final int defaultPrefetchChunks; | ||
private final QueryOptions defaultQueryOptions; | ||
|
||
private final Clock clock; | ||
|
||
@GuardedBy("lock") | ||
private boolean isValid = true; | ||
|
||
|
@@ -416,6 +425,7 @@ void initTransaction() { | |
this.defaultQueryOptions = builder.defaultQueryOptions; | ||
this.span = builder.span; | ||
this.executorProvider = builder.executorProvider; | ||
this.clock = builder.clock == null ? new Clock() : builder.clock; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: see above, we could remove this null check by setting a default in the builder. Otherwise, prefer the use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
} | ||
|
||
@Override | ||
|
@@ -826,6 +836,7 @@ CloseableIterator<PartialResultSet> startStream(@Nullable ByteString resumeToken | |
SpannerRpc.StreamingCall call = | ||
rpc.read( | ||
builder.build(), stream.consumer(), session.getOptions(), isRouteToLeader()); | ||
session.markUsed(clock.instant()); | ||
olavloite marked this conversation as resolved.
Show resolved
Hide resolved
|
||
call.request(prefetchChunks); | ||
stream.setCall(call, /* withBeginTransaction = */ builder.getTransaction().hasBegin()); | ||
return stream; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Copyright 2023 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.google.cloud.spanner; | ||
|
||
import org.threeten.bp.Instant; | ||
|
||
/** | ||
* Wrapper around current time so that we can fake it in tests. TODO(user): Replace with Java 8 | ||
* Clock. | ||
*/ | ||
class Clock { | ||
Instant instant() { | ||
return Instant.now(); | ||
} | ||
} |
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: maybe set this by default to
= new Clock()
and remove the null check in the AbstractReadContext constructor?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.
Sure, this is the clock field within the builder class. I have set it to
= new Clock()
by default at the member definition (L402)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 actually meant setting this one (so
Builder.clock
) by default to= new Clock()
. That way, the clock in theAbstractReadContext
can be final.