korlov42 commented on code in PR #945:
URL: https://github.com/apache/ignite-3/pull/945#discussion_r930671816
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionImpl.java:
##########
@@ -219,6 +219,8 @@ public CompletableFuture<long[]>
executeBatchAsync(@Nullable Transaction transac
if (ex instanceof CancellationException) {
qryFut.cancel(false);
}
+
+ qryFut.join().closeAsync();
Review Comment:
why do you put this line?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -366,7 +370,7 @@ public CompletableFuture<Void> closeAsync() {
);
});
- stage.whenComplete((cur, ex) -> {
+ stage = stage.whenComplete((cur, ex) -> {
Review Comment:
the idea behind is that you return future with attached cancellation handler
to the client. Now you return the future of the handler to the client, and this
doesn't make any sense
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -424,7 +436,7 @@ private void onError(RemoteException ex) {
private void onNodeLeft(String nodeId) {
remoteFragmentInitCompletion.entrySet().stream().filter(e ->
nodeId.equals(e.getKey().nodeId()))
- .forEach(e -> e.getValue().completeExceptionally(new
IgniteInternalException("asddd")));
+ .forEach(e -> e.getValue().completeExceptionally(new
IgniteInternalException("Node left, nodeId=" + nodeId)));
Review Comment:
```suggestion
.forEach(e -> e.getValue().completeExceptionally(new
IgniteInternalException("Node left the cluster [nodeId=" + nodeId + ']')));
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java:
##########
@@ -264,6 +273,48 @@ void before() throws Exception {
.get(1, TimeUnit.SECONDS);
}
+ /**
+ * Checks inner transactions are initialized correctly.
+ */
+ @Test
+ public void testInnerTxInitiated() throws Exception {
+ SessionId sesId = queryProc.createSession(new PropertiesHolder() {
+ @Override
Review Comment:
`PropertiesHolder.holderFor(Map.of())`
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -159,10 +167,20 @@ public void dml() {
IgniteSql sql = igniteSql();
Review Comment:
Oh, I see there is something similar in implicitTransactionsWithDml test.
Let's split this onto several smaller tests
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java:
##########
@@ -159,10 +167,20 @@ public void dml() {
IgniteSql sql = igniteSql();
Review Comment:
Could you please add the following case for both Sync and Async embedded API
implementation:
- start explicit transaction
- insert some rows to a table
- rollback transaction
- verify rows were not inserted
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionContext.java:
##########
@@ -332,6 +339,18 @@ public interface RunnableX {
void run() throws Throwable;
}
+ /** Transaction for current context. */
+ public InternalTransaction transaction() {
+ return tx;
+ }
+
+ /** Transaction. */
+ public void transaction(InternalTransaction tx) {
Review Comment:
do we really need such setter? Looks like it is not used anywhere
--
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]
