chore: add getter for database role in DatabaseClient and BatchClient #2029
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… for beam unit tests
rajatbhatta
suggested changes
Sep 22, 2022
| * determines the access permissions that a connection has. This can for example be used to create | ||
| * connections that are only permitted to access certain tables. | ||
| */ | ||
| String getDatabaseRole(); |
Contributor
There was a problem hiding this comment.
Suggested change
| String getDatabaseRole(); | |
| default String getDatabaseRole() { | |
| throw new UnsupportedOperationException("method should be overwritten"); | |
| } |
I think we should provide a default implementation for this method in the interface, so that it isn't a breaking change.
|
|
||
| private final SessionPoolOptions options; | ||
| private final SettableFuture<Dialect> dialect = SettableFuture.create(); | ||
|
|
Contributor
There was a problem hiding this comment.
nit: remove extra newline.
| Clock clock) { | ||
| return createPool( | ||
| poolOptions, | ||
| "", |
Contributor
There was a problem hiding this comment.
Should we pass in null here?
Suggested change
| "", | |
| null, |
| } | ||
| } | ||
|
|
||
| String getDatabaseRole() { |
Contributor
There was a problem hiding this comment.
Suggested change
| String getDatabaseRole() { | |
| public String getDatabaseRole() { |
| Clock clock, MetricRegistry metricRegistry, List<LabelValue> labelValues) { | ||
| return SessionPool.createPool( | ||
| options, | ||
| "", |
Contributor
There was a problem hiding this comment.
Suggested change
| "", | |
| null, |
| try (ResultSet rs = | ||
| dbClient.singleUse().executeQuery(Statement.of("SELECT COUNT(*) as cnt FROM T"))) { | ||
| assertTrue(rs.next()); | ||
| assertThat(dbClient.getDatabaseRole()).isEqualTo(dbRoleParent); |
Contributor
There was a problem hiding this comment.
Suggested change
| assertThat(dbClient.getDatabaseRole()).isEqualTo(dbRoleParent); | |
| assertEquals(dbClient.getDatabaseRole(), dbRoleParent); |
nit: change to assertEquals.
Contributor
|
@rahul2393 : Can we do the following as well?
|
cc008d9 to
c3c9e81
Compare
rajatbhatta
reviewed
Sep 22, 2022
| import static com.google.cloud.spanner.MetricRegistryConstants.NUM_WRITE_SESSIONS; | ||
| import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS; | ||
| import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS_WITH_TYPE; | ||
| import static com.google.cloud.spanner.MetricRegistryConstants.*; |
Contributor
There was a problem hiding this comment.
Suggested change
| import static com.google.cloud.spanner.MetricRegistryConstants.*; | |
| import static com.google.cloud.spanner.MetricRegistryConstants.NUM_IN_USE_SESSIONS; | |
| import static com.google.cloud.spanner.MetricRegistryConstants.NUM_READ_SESSIONS; | |
| import static com.google.cloud.spanner.MetricRegistryConstants.NUM_SESSIONS_BEING_PREPARED; | |
| import static com.google.cloud.spanner.MetricRegistryConstants.NUM_WRITE_SESSIONS; | |
| import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS; | |
| import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS_WITH_TYPE; |
Google style requires us to never use wildcard imports: https://google.github.io/styleguide/javaguide.html
rajatbhatta
reviewed
Sep 22, 2022
| } | ||
|
|
||
| @Test | ||
| public void testDatabaseRole() throws Exception { |
Contributor
There was a problem hiding this comment.
Suggested change
| public void testDatabaseRole() throws Exception { | |
| public void testGetDatabaseRole() throws Exception { |
26e1438 to
ee9b14b
Compare
rajatbhatta
reviewed
Sep 22, 2022
google-cloud-spanner/src/test/java/com/google/cloud/spanner/BatchClientImplTest.java
Outdated
Show resolved
Hide resolved
…tchClientImplTest.java
rajatbhatta
approved these changes
Sep 22, 2022
Contributor
There was a problem hiding this comment.
LGTM.
@olavloite: Can you please take a look at this PR once? Thanks!
olavloite
approved these changes
Sep 28, 2022
Collaborator
olavloite
left a comment
There was a problem hiding this comment.
LGTM (with a tiny nit on the Javadoc)
| BatchReadOnlyTransaction batchReadOnlyTransaction(BatchTransactionId batchTransactionId); | ||
|
|
||
| /** | ||
| * Returns the {@link DatabaseRole} used by the client connection. The database role that is used |
Collaborator
There was a problem hiding this comment.
nit: replace the {@link DatabaseRole} with just database role. DatabaseRole is not a valid class name, so it cannot be linked.
| } | ||
|
|
||
| /** | ||
| * Returns the {@link DatabaseRole} used by the client connection. The database role that is used |
ansh0l
approved these changes
Sep 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.