zstan commented on code in PR #13126:
URL: https://github.com/apache/ignite/pull/13126#discussion_r3239169940
##########
docs/_docs/sql-reference/transactions.adoc:
##########
@@ -31,7 +31,7 @@ SAVEPOINT savepointName
=== Description
-`SAVEPOINT` can be used only inside an explicit `PESSIMISTIC` transaction.
+`SAVEPOINT` can be used only inside an explicit transaction.
Review Comment:
the same
##########
docs/_docs/key-value-api/transactions.adoc:
##########
@@ -95,7 +95,7 @@ It is critical that an Ignite Transaction should be `closed`
regardless of its c
Savepoints allow you to mark an intermediate state inside an explicit
transaction and later roll back only the changes made after that point.
They are useful when a transaction contains several logical steps and one of
the later steps can be discarded without rolling back the whole transaction.
-Ignite supports savepoints only for explicit `PESSIMISTIC` transactions.
+Ignite supports savepoints only for explicit transactions.
Review Comment:
We never told about implicit tx, thus it is smth inner (implementation
details) do you think - it correct to mention "explicit transactions" ? If so -
we need to have a resposne - "what is - implicit tx" ?
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/transactions/TxSavepointParameterizedTest.java:
##########
@@ -402,6 +441,48 @@ public void testRollbackToSavepoint() {
assertEquals(Integer.valueOf(3), cache0.get(key3));
}
+ /**
+ * @throws Exception If failed.
Review Comment:
Such comment is uninformative - stay it empty or fill smth informative
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/transactions/TxSavepointPessimisticTest.java:
##########
@@ -30,14 +29,8 @@
import org.apache.ignite.transactions.TransactionException;
import org.junit.Test;
-import static org.apache.ignite.transactions.TransactionConcurrency.OPTIMISTIC;
-import static
org.apache.ignite.transactions.TransactionConcurrency.PESSIMISTIC;
-import static
org.apache.ignite.transactions.TransactionIsolation.READ_COMMITTED;
-import static
org.apache.ignite.transactions.TransactionIsolation.REPEATABLE_READ;
-
/**
* Tests transaction savepoint functionality.
- * Currently, savepoint API is supported only for pessimistic transactions.
*/
public class TxSavepointPessimisticTest extends GridCommonAbstractTest {
Review Comment:
```suggestion
public class TxSavepointItTest extends GridCommonAbstractTest {
```
or TxSavepointIntegrationTest
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/transactions/TxSavepointPessimisticTest.java:
##########
Review Comment:
```suggestion
public void testRollabackSeveralSavepoints() {
```
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/jdbc/JdbcThinConnectionSavepointTest.java:
##########
@@ -29,17 +29,36 @@
import org.apache.ignite.configuration.SqlConfiguration;
import org.apache.ignite.configuration.TransactionConfiguration;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
import static org.apache.ignite.testframework.GridTestUtils.assertThrows;
/** Savepoint tests for thin JDBC connection. */
+@RunWith(Parameterized.class)
public class JdbcThinConnectionSavepointTest extends AbstractJdbcTest {
/** */
private static final String TBL = "SAVEPOINT_TEST_TABLE";
/** JDBC URL. */
- private static final String SAVEPOINT_URL = URL + "?queryEngine=" +
CalciteQueryEngineConfiguration.ENGINE_NAME +
- "&transactionConcurrency=PESSIMISTIC";
+ private static final String SAVEPOINT_URL = URL + "?queryEngine=" +
CalciteQueryEngineConfiguration.ENGINE_NAME;
+
+ /** Transaction concurrency URL parameter. */
+ @Parameter
+ public String txConcurrencyParam;
+
+ /**
+ * @return Test parameters.
+ */
+ @Parameters(name = "{0}")
+ public static Iterable<Object[]> testData() {
+ return Arrays.asList(new Object[][] {
+ {"&transactionConcurrency=PESSIMISTIC"},
Review Comment:
```suggestion
{"&transactionConcurrency=" +
TransactionConcurrency.PESSIMISTIC},
```
or more clear - concut it in function below : `Connection connection()`
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/transactions/TxSavepointParameterizedTest.java:
##########
@@ -402,6 +441,48 @@ public void testRollbackToSavepoint() {
assertEquals(Integer.valueOf(3), cache0.get(key3));
}
+ /**
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testRollbackToSavepointForOptimisticTransaction() throws
Exception {
Review Comment:
what`s the difference between TxSavepointPessimisticTest and
TxSavepointParameterizedTest, do we really need 2 different classes for now ?
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/transactions/TxSavepointPessimisticTest.java:
##########
@@ -129,7 +105,7 @@ public void testRollabackSeveralSavepoints() throws
Exception {
*/
@Test
public void testDuplicateSavepointWithoutOverwriteThrows() throws
Exception {
Review Comment:
```suggestion
public void testDuplicateSavepointWithoutOverwriteThrows() {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
