Skip to content

spanner-jdbc: Step 18 - ConnectionImpl - #5895

Merged
olavloite merged 4 commits into
googleapis:spanner-jdbcfrom
olavloite:spanner-jdbc-18-connection-impl
Jul 30, 2019
Merged

spanner-jdbc: Step 18 - ConnectionImpl#5895
olavloite merged 4 commits into
googleapis:spanner-jdbcfrom
olavloite:spanner-jdbc-18-connection-impl

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Adds the implementation class of the internal Spanner connection API. JDBC connections act as wrappers around this class. A ConnectionImpl instance is used to interact with Cloud Spanner through several different types of transactions or batches. There can be at most one transaction or batch active at any one time on a connection.

This PR will have build errors as it depends on step 15 (ConnectionOptions), but also on the different types of transaction classes that will be brought in through separate PRs.

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2019
@Override
public Thread newThread(Runnable r) {
Thread t = new Thread(r);
t.setDaemon(true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we also add a qualifying name?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added.

this.spannerPool = SpannerPool.INSTANCE;
this.options = options;
this.spanner = spannerPool.getSpanner(options, this);
this.ddlClient = DdlClient.newBuilder().setDatabaseAdminClient(spanner.getDatabaseAdminClient())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd move this logic to a private method, createDdlClient

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved to a separate method, and moved the assignment to the end of the constructor to ensure that all fields have been initialized.

}

@Override
public StatementResult statementSetAutocommit(Boolean autocommit) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should autocommit be autoCommit? WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(not sure if this is an already defined naming convention elsewhere)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Both are used. I have a slight preference for autocommit, but I don't feel very strongly about this.

JDBC uses autoCommit. SQL Server uses auto-commit. PostgreSQL and MySQL use autocommit in their documentation.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

One reason to keep this as autocommit is that we have defined the variable AUTOCOMMIT_DML_MODE, which indicates that the driver assumes autocommit to be one word.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SGTM

@olavloite
olavloite requested a review from kolea2July 28, 2019 21:34
@olavloite
olavloiteforce-pushed the spanner-jdbc-18-connection-impl branch from 7fbf46a to fababa6CompareJuly 30, 2019 05:41
@olavloite
olavloite merged commit 9cc5e1c into googleapis:spanner-jdbcJul 30, 2019
olavloite added a commit to olavloite/google-cloud-java that referenced this pull request Aug 5, 2019
* add ConnectionImpl
* fix CR year
* moved DdlClient creation to separate method
* updated changed class reference
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@olavloite@kolea2@googlebot