Spanner expose databaseId provider bean #1530

ChengyuanZhao · 2019-03-14T22:09:19Z

related #1525
allows bean for Database ID provider to support multiple instances or databases

meltsufin · 2019-03-15T13:39:24Z

...main/java/org/springframework/cloud/gcp/data/spanner/core/admin/DatabaseUtilityProvider.java

+ *
+ * @since 1.2
+ */
+public interface DatabaseUtilityProvider<T> {


This looks identical to java.util.function.Supplier except for the name. Why not just use Supplier in its place?

Yea I actually prefer using those general "functional" classes. Will change.

meltsufin · 2019-03-15T13:45:11Z

docs/src/main/asciidoc/spanner.adoc

+[source,java]
+----
+    @Bean
+    public DatabaseUtilityProvider<DatabaseId> databaseIdProvider() {


I thought we discussed and agreed on using something like DatabaseClientProvider. It's more flexible in a sense that it allows the app to connect to Spanner instances even in different projects.
You can always implement a DatabaseClientProvider that works with DatabaseId.

@meltsufin , so there is a provider bean for DatabaseClient exposed. However the way DatabaseClients receive their instance/database/project id is via the DatabaseId class. furthermore there is a DatabaseAdminClient that also uses the DatabaseId
so it made sense to also make a provider bean available for DatabaseId
(emphasizing that DatabaseClient provider bean is also exposed)

Updated the refdoc to emphasize that a bean for Supplier of DatabaseClient is also available and an option to set.

Speaking of Suppliers, should we go back and remove CredentialProvider and ProjectIDProvider and just use Suppler of Credentials and Supplier of ProjectID ?

meltsufin

Some more review.

meltsufin · 2019-03-15T15:50:03Z

...va/org/springframework/cloud/gcp/data/spanner/core/admin/CachingDatabaseUtilityProvider.java

+ * @param <U> the type of objects this provider bases its products on.
+ * @author Chengyuan Zhao
+ */
+public class CachingDatabaseUtilityProvider<T, U> implements Supplier<T> {


I think this is too generic. The specificity of the name of the class does not reflect its function. What it does seems very generic and not specific to "Database".

removed a type parameter, PTAL

meltsufin · 2019-03-15T15:51:25Z

...java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerDatabaseAdminTemplate.java

 	 */
 	public SpannerDatabaseAdminTemplate(DatabaseAdminClient databaseAdminClient,
-			DatabaseClient databaseClient,
-			DatabaseId databaseId) {
+										Supplier<DatabaseClient> databaseClientProvider,


Why would you need this, if you can compute it from the databaseIdProvider?

If the user had chosen to provide his own DatabaseClient supplier then that wouldn't be right (for example if he sets things that aren't in DatabaseId). Also if we just use the DAtabaseIdProvider we would need to create new instances of DatabaseClient in here (instead of using existing ones) and also have a Spanner.class on hand.

codecov · 2019-03-15T16:25:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5c6ac41). Click here to learn what that means.
The diff coverage is 94.44%.

@@            Coverage Diff            @@
##             master    #1530   +/-   ##
=========================================
  Coverage          ?   69.22%           
  Complexity        ?     1633           
=========================================
  Files             ?      238           
  Lines             ?     6281           
  Branches          ?      647           
=========================================
  Hits              ?     4348           
  Misses            ?     1648           
  Partials          ?      285

Flag	Coverage Δ	Complexity Δ
#unittests	`69.22% <94.44%> (?)`	`1633 <10> (?)`

Impacted Files	Coverage Δ	Complexity Δ
...nner/core/ReadWriteTransactionSpannerTemplate.java	`70% <ø> (ø)`	`4 <0> (?)`
...anner/core/ReadOnlyTransactionSpannerTemplate.java	`55.55% <ø> (ø)`	`3 <0> (?)`
...anner/core/admin/SpannerDatabaseAdminTemplate.java	`83.07% <100%> (ø)`	`20 <3> (?)`
...configure/spanner/GcpSpannerAutoConfiguration.java	`70.9% <100%> (ø)`	`1 <0> (?)`
...nner/core/admin/CachingDatabaseClientProvider.java	`100% <100%> (ø)`	`2 <2> (?)`
...er/SpannerTransactionManagerAutoConfiguration.java	`80% <100%> (ø)`	`1 <0> (?)`
...p/data/spanner/core/SpannerTransactionManager.java	`69.69% <100%> (ø)`	`13 <1> (?)`
...k/cloud/gcp/data/spanner/core/SpannerTemplate.java	`73.07% <81.81%> (ø)`	`73 <4> (?)`

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 5c6ac41...166b8c8. Read the comment docs.

codecov · 2019-03-15T16:25:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5c6ac41). Click here to learn what that means.
The diff coverage is 94.44%.

@@            Coverage Diff            @@
##             master    #1530   +/-   ##
=========================================
  Coverage          ?   69.22%           
  Complexity        ?     1633           
=========================================
  Files             ?      238           
  Lines             ?     6281           
  Branches          ?      647           
=========================================
  Hits              ?     4348           
  Misses            ?     1648           
  Partials          ?      285

Flag	Coverage Δ	Complexity Δ
#unittests	`69.22% <94.44%> (?)`	`1633 <10> (?)`

Impacted Files	Coverage Δ	Complexity Δ
...nner/core/ReadWriteTransactionSpannerTemplate.java	`70% <ø> (ø)`	`4 <0> (?)`
...anner/core/ReadOnlyTransactionSpannerTemplate.java	`55.55% <ø> (ø)`	`3 <0> (?)`
...a/spanner/core/admin/CachingComposingSupplier.java	`100% <100%> (ø)`	`2 <2> (?)`
...anner/core/admin/SpannerDatabaseAdminTemplate.java	`83.07% <100%> (ø)`	`20 <3> (?)`
...configure/spanner/GcpSpannerAutoConfiguration.java	`70.9% <100%> (ø)`	`1 <0> (?)`
...er/SpannerTransactionManagerAutoConfiguration.java	`80% <100%> (ø)`	`1 <0> (?)`
...p/data/spanner/core/SpannerTransactionManager.java	`69.69% <100%> (ø)`	`13 <1> (?)`
...k/cloud/gcp/data/spanner/core/SpannerTemplate.java	`73.07% <81.81%> (ø)`	`73 <4> (?)`

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 5c6ac41...1eaf40c. Read the comment docs.

meltsufin · 2019-03-15T17:10:53Z

...ava/org/springframework/cloud/gcp/data/spanner/core/admin/CachingDatabaseClientProvider.java

+ * @param <U> the type of objects this provider bases its products on.
+ * @author Chengyuan Zhao
+ */
+public class CachingDatabaseClientProvider<U> implements Supplier<DatabaseClient> {


I don't think there's much value in this U type parameter either.
I would just call this class DatabaseIdBasedDatabaseClientProvider and it doesn't need any generics at all. It just needs the DatabaseIdProvider and a Spanner instance.
Why does this class even need to be public?

I don't think there's a reason to make it that restrictive. There's no reason U can't be the Spanner object in another use-case or a just a string specifying the instance ID.
I don't think we gain any simplification by being more restrictive here, actually.

In that case, you might be better off going back to the most generic design as you had before, and just renaming this class to CachingSupplier<INPUT, OUTPUT> implements Supplier<OUTPUT>.

sounds good! changes made

docs/src/main/asciidoc/spanner.adoc

...ava/org/springframework/cloud/gcp/data/spanner/core/ReadWriteTransactionSpannerTemplate.java

...ain/java/org/springframework/cloud/gcp/data/spanner/core/admin/CachingComposingSupplier.java

dzou · 2019-03-15T17:40:54Z

.../src/main/java/org/springframework/cloud/gcp/data/spanner/core/admin/DatabaseIdProvider.java

+ *
+ * @since 1.2
+ */
+public interface DatabaseIdProvider extends Supplier<DatabaseId> {


What was the reason for creating this wrapper around Supplier<DatabaseId>? Is it better to just use Supplier<DatabaseId> in the code?

See a comment further up from Mike :P

...java/org/springframework/cloud/gcp/data/spanner/core/admin/SpannerDatabaseAdminTemplate.java

dmitry-s

We probably should add an example or integration test for this feature.

ChengyuanZhao · 2019-03-15T19:28:27Z

@dmitry-s Done !! PTAL

dmitry-s

Looks good!

Spanner expose databaseId provider bean

f87f453

ChengyuanZhao added the spanner label Mar 14, 2019

ChengyuanZhao self-assigned this Mar 14, 2019

ChengyuanZhao requested review from dmitry-s and meltsufin March 14, 2019 22:09

meltsufin suggested changes Mar 15, 2019

View reviewed changes

pr comments

1ecfde8

meltsufin requested a review from dzou March 15, 2019 15:26

added interface DatabaseIdProvider

fe4919c

meltsufin reviewed Mar 15, 2019

View reviewed changes

pr comment

166b8c8

meltsufin reviewed Mar 15, 2019

View reviewed changes

ChengyuanZhao added 2 commits March 15, 2019 13:32

pr comment

10d522d

merge

24edd17

dzou reviewed Mar 15, 2019

View reviewed changes

indent fix

49b3e2a

dmitry-s suggested changes Mar 15, 2019

View reviewed changes

added integration test

c7cb455

ChengyuanZhao added 2 commits March 15, 2019 15:29

checkstyle

8256dc8

remove unnecessary

1eaf40c

meltsufin approved these changes Mar 15, 2019

View reviewed changes

dmitry-s approved these changes Mar 15, 2019

View reviewed changes

ChengyuanZhao merged commit 16f9aad into master Mar 15, 2019

ChengyuanZhao deleted the spanner-template-provider branch March 15, 2019 19:51

meltsufin mentioned this pull request Mar 21, 2019

SpannerTemplate support choosing among multiple connections/instances #1525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spanner expose databaseId provider bean #1530

Spanner expose databaseId provider bean #1530

ChengyuanZhao commented Mar 14, 2019

meltsufin Mar 15, 2019

ChengyuanZhao Mar 15, 2019

meltsufin Mar 15, 2019

ChengyuanZhao Mar 15, 2019

ChengyuanZhao Mar 15, 2019

meltsufin left a comment

meltsufin Mar 15, 2019

ChengyuanZhao Mar 15, 2019

meltsufin Mar 15, 2019

ChengyuanZhao Mar 15, 2019 •

edited

Loading

codecov bot commented Mar 15, 2019

codecov bot commented Mar 15, 2019 •

edited

Loading

meltsufin Mar 15, 2019

ChengyuanZhao Mar 15, 2019

meltsufin Mar 15, 2019

ChengyuanZhao Mar 15, 2019

dzou Mar 15, 2019

ChengyuanZhao Mar 15, 2019

dmitry-s left a comment

ChengyuanZhao commented Mar 15, 2019

dmitry-s left a comment

Spanner expose databaseId provider bean #1530

Spanner expose databaseId provider bean #1530

Conversation

ChengyuanZhao commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengyuanZhao Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2019

Codecov Report

codecov bot commented Mar 15, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-s left a comment

Choose a reason for hiding this comment

ChengyuanZhao commented Mar 15, 2019

dmitry-s left a comment

Choose a reason for hiding this comment

ChengyuanZhao Mar 15, 2019 •

edited

Loading

codecov bot commented Mar 15, 2019 •

edited

Loading