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.

Sign up for GitHub

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

Jump to bottom

feat: support Savepoint #1212

Merged
merged 3 commits into from
May 26, 2023
Merged

feat: support Savepoint #1212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fd60ef
feat: support Savepoint
olavloite Apr 25, 2023
e99bda1
fix: clirr check
olavloite Apr 26, 2023
3ed8ee0
Merge branch 'main' into savepoint
olavloite May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions clirr-ignored-differences.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,14 @@
<className>com/google/cloud/spanner/jdbc/CloudSpannerJdbcConnection</className>
<method>com.google.cloud.spanner.Dialect getDialect()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/jdbc/CloudSpannerJdbcConnection</className>
<method>com.google.cloud.spanner.connection.SavepointSupport getSavepointSupport()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/jdbc/CloudSpannerJdbcConnection</className>
<method>void setSavepointSupport(com.google.cloud.spanner.connection.SavepointSupport)</method>
</difference>
</differences>
22 changes: 0 additions & 22 deletions src/main/java/com/google/cloud/spanner/jdbc/AbstractJdbcConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.sql.SQLException;
import java.sql.SQLWarning;
import java.sql.SQLXML;
import java.sql.Savepoint;
import java.sql.Struct;
import java.util.Properties;
import java.util.concurrent.Executor;
Expand All @@ -42,7 +41,6 @@ abstract class AbstractJdbcConnection extends AbstractJdbcWrapper
"Only isolation level TRANSACTION_SERIALIZABLE is supported";
private static final String ONLY_CLOSE_ALLOWED =
"Only holdability CLOSE_CURSORS_AT_COMMIT is supported";
private static final String SAVEPOINTS_UNSUPPORTED = "Savepoints are not supported";
private static final String SQLXML_UNSUPPORTED = "SQLXML is not supported";
private static final String STRUCTS_UNSUPPORTED = "Structs are not supported";
private static final String ABORT_UNSUPPORTED = "Abort is not supported";
Expand Down Expand Up @@ -163,26 +161,6 @@ public void clearWarnings() throws SQLException {
lastWarning = null;
}

@Override
public Savepoint setSavepoint() throws SQLException {
return checkClosedAndThrowUnsupported(SAVEPOINTS_UNSUPPORTED);
}

@Override
public Savepoint setSavepoint(String name) throws SQLException {
return checkClosedAndThrowUnsupported(SAVEPOINTS_UNSUPPORTED);
}

@Override
public void rollback(Savepoint savepoint) throws SQLException {
checkClosedAndThrowUnsupported(SAVEPOINTS_UNSUPPORTED);
}

@Override
public void releaseSavepoint(Savepoint savepoint) throws SQLException {
checkClosedAndThrowUnsupported(SAVEPOINTS_UNSUPPORTED);
}

@Override
public SQLXML createSQLXML() throws SQLException {
return checkClosedAndThrowUnsupported(SQLXML_UNSUPPORTED);
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/google/cloud/spanner/jdbc/CloudSpannerJdbcConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.cloud.spanner.ResultSet;
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.connection.AutocommitDmlMode;
import com.google.cloud.spanner.connection.SavepointSupport;
import com.google.cloud.spanner.connection.TransactionMode;
import java.sql.Connection;
import java.sql.SQLException;
Expand Down Expand Up @@ -256,6 +257,12 @@ default String getStatementTag() throws SQLException {
*/
void setRetryAbortsInternally(boolean retryAbortsInternally) throws SQLException;

/** Returns the current savepoint support for this connection. */
SavepointSupport getSavepointSupport() throws SQLException;

/** Sets how savepoints should be supported on this connection. */
void setSavepointSupport(SavepointSupport savepointSupport) throws SQLException;

/**
* Writes the specified mutation directly to the database and commits the change. The value is
* readable after the successful completion of this method. Writing multiple mutations to a
Expand Down
66 changes: 66 additions & 0 deletions src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.connection.AutocommitDmlMode;
import com.google.cloud.spanner.connection.ConnectionOptions;
import com.google.cloud.spanner.connection.SavepointSupport;
import com.google.cloud.spanner.connection.TransactionMode;
import com.google.common.collect.Iterators;
import java.sql.Array;
Expand All @@ -33,6 +34,7 @@
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Savepoint;
import java.sql.Statement;
import java.sql.Timestamp;
import java.util.HashMap;
Expand Down Expand Up @@ -402,6 +404,70 @@ public String getSchema() throws SQLException {
return "";
}

@Override
public SavepointSupport getSavepointSupport() throws SQLException {
checkClosed();
return getSpannerConnection().getSavepointSupport();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we intentionally skipped checkClosed(); validation in this method? If yes, I didn't get why we won't need the check here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really intentionally. Added it to be consistent with the other methods.

}

@Override
public void setSavepointSupport(SavepointSupport savepointSupport) throws SQLException {
checkClosed();
try {
getSpannerConnection().setSavepointSupport(savepointSupport);
} catch (SpannerException e) {
throw JdbcSqlExceptionFactory.of(e);
}
}

@Override
public Savepoint setSavepoint() throws SQLException {
checkClosed();
try {
JdbcSavepoint savepoint = JdbcSavepoint.unnamed();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's your take on inversion of control ? Are you intentionally using JdbcSavepoint since you require the static methods and the internalGetSavepointName() method?

Usually with interfaces we don't expose the implementation class at time of dependency injection. Over here it's not really dependency injection, but would it not be better to use just Savepoint interface instead of referencing JdbcSavepoint class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to use the JdbcSavepoint class instead of the Savepoint interface here, because we need access to the internalGetSavepointName() method. We cannot use the Savepoint#getSavepointName() method, because that method will throw an error for unnamed savepoints. The 'throw-an-error' behavior is part of the JDBC specification, so it is not something that we can choose to implement differently.

getSpannerConnection().savepoint(savepoint.internalGetSavepointName());
return savepoint;
} catch (SpannerException e) {
throw JdbcSqlExceptionFactory.of(e);
}
}

@Override
public Savepoint setSavepoint(String name) throws SQLException {
checkClosed();
try {
JdbcSavepoint savepoint = JdbcSavepoint.named(name);
getSpannerConnection().savepoint(savepoint.internalGetSavepointName());
return savepoint;
} catch (SpannerException e) {
throw JdbcSqlExceptionFactory.of(e);
}
}

@Override
public void rollback(Savepoint savepoint) throws SQLException {
checkClosed();
JdbcPreconditions.checkArgument(savepoint instanceof JdbcSavepoint, savepoint);
JdbcSavepoint jdbcSavepoint = (JdbcSavepoint) savepoint;
try {
getSpannerConnection().rollbackToSavepoint(jdbcSavepoint.internalGetSavepointName());
Comment on lines +451 to +453
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar comments on usage of JdbcSavepoint class instead of Savepoint interface as above methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above: We need access to the internalGetSavepointName() method, as the Savepoint#getSavepointName() could throw an error if the given savepoint is an unnamed savepoint.

} catch (SpannerException e) {
throw JdbcSqlExceptionFactory.of(e);
}
}

@Override
public void releaseSavepoint(Savepoint savepoint) throws SQLException {
checkClosed();
JdbcPreconditions.checkArgument(savepoint instanceof JdbcSavepoint, savepoint);
JdbcSavepoint jdbcSavepoint = (JdbcSavepoint) savepoint;
try {
getSpannerConnection().releaseSavepoint(jdbcSavepoint.internalGetSavepointName());
} catch (SpannerException e) {
throw JdbcSqlExceptionFactory.of(e);
}
}

@Override
public Timestamp getCommitTimestamp() throws SQLException {
checkClosed();
Expand Down
58 changes: 58 additions & 0 deletions src/main/java/com/google/cloud/spanner/jdbc/JdbcSavepoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* 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.jdbc;

import java.sql.SQLException;
import java.sql.Savepoint;
import java.util.concurrent.atomic.AtomicInteger;

class JdbcSavepoint implements Savepoint {
private static final AtomicInteger COUNTER = new AtomicInteger();

static JdbcSavepoint named(String name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason we are preferring static methods for instantiation instead of adding constructors for creating named and unnamed instances? Wouldn't the constructor be a more natural way for creating instance instead of static methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gives more readable code where the savepoints are created, especially as the behavior of named and unnamed savepoints are slightly different. Internally, we always need to create savepoints with a name, as that is the only thing that Cloud Spanner supports. This way we make clear that one is a named savepoint, and the other is unnamed, even though both internally have a name.

return new JdbcSavepoint(-1, name);
}

static JdbcSavepoint unnamed() {
int id = COUNTER.incrementAndGet();
return new JdbcSavepoint(id, String.format("s_%d", id));
}

private final int id;
private final String name;

private JdbcSavepoint(int id, String name) {
this.id = id;
this.name = name;
}

@Override
public int getSavepointId() throws SQLException {
JdbcPreconditions.checkState(this.id >= 0, "This is a named savepoint");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion : It might be more readable to track named vs un-named through an explicit boolean variable say isNamed . Using the the ID's state to determine this seems like an over-use of ID's value beyond what its meant for. For example - If for any reason we change this ID's type from integer to String, it ideally shouldn't impact the logic to determine named vs un-named. But in this case we will need to modify the logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't feel it's really necessary given the size of the class. Also; changing the type of ID from integer to anything else will not be possible, as the type is specified by the JDBC API (that is; getSavepointId must return an int).

return id;
}

@Override
public String getSavepointName() throws SQLException {
JdbcPreconditions.checkState(this.id < 0, "This is an unnamed savepoint");
return name;
}

String internalGetSavepointName() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Why is this method being added to an implementation when the interface Savepoint is not defining any such method?
  2. Why are we prefixing this with internal ? Wouldn't it be better to call this getName which acts like a getter method for existing member name?
  3. Why are we adding this method when there is already getSavepointName() method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need a separate method from getSavepointName() because:

  1. JDBC specifies that getSavepointName() must throw an error if it is called on an unnamed savepoint.
  2. We internally need to get the name of a savepoint in order to set it on the underlying connection. For an unnamed savepoint, this will be a generated name that is only accessible internally in the package, and not to the outside world.
  3. internalGetSavepointName() is the internal (package-private) version of the getSavepointName() method that returns the name regardless whether the savepoint is named or unnamed. This is used internally to set the savepoint name on the underlying connection.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation.

return name;
}
}
47 changes: 47 additions & 0 deletions src/test/java/com/google/cloud/spanner/jdbc/JdbcSavepointTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* 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.jdbc;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

import java.sql.SQLException;
import java.sql.Savepoint;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class JdbcSavepointTest {

@Test
public void testNamed() throws SQLException {
Savepoint savepoint = JdbcSavepoint.named("test");
assertEquals("test", savepoint.getSavepointName());
assertThrows(SQLException.class, savepoint::getSavepointId);
}

@Test
public void testUnnamed() throws SQLException {
Savepoint savepoint = JdbcSavepoint.unnamed();
assertTrue(
String.format("Savepoint id: %d", savepoint.getSavepointId()),
savepoint.getSavepointId() > 0);
assertThrows(SQLException.class, savepoint::getSavepointName);
}
}