Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
leonardBang merged PR #23564: URL: https://github.com/apache/flink/pull/23564 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1781504928 Hi @leonardBang, do you have time to help review it? Thanks. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1780717657 Hi @fsk119, could you help review it when you have time? Thanks a lot. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
WencongLiu commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1776956979 cc @fsk119 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
WencongLiu commented on code in PR #23564:
URL: https://github.com/apache/flink/pull/23564#discussion_r1369952969
##
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/OperationRelatedITCase.java:
##
@@ -119,18 +119,25 @@ List submitOperation() throws Exception {
SessionHandle sessionHandle = new
SessionHandle(UUID.fromString(sessionHandleId));
assertThat(SQL_GATEWAY_SERVICE_EXTENSION.getSessionManager().getSession(sessionHandle))
.isNotNull();
+
+OneShotLatch startLatch = new OneShotLatch();
+Thread main = Thread.currentThread();
OperationHandle operationHandle =
SQL_GATEWAY_SERVICE_EXTENSION
.getService()
.submitOperation(
sessionHandle,
() -> {
try {
-TimeUnit.SECONDS.sleep(10);
+startLatch.trigger();
+// keep operation in RUNNING state in
response to cancel
+// or close operations.
+main.join();
} catch (InterruptedException ignored) {
}
return NotReadyResult.INSTANCE;
});
+startLatch.await();
Review Comment:
Good point. I overlooked the fact that `OneShotLatch#await()` will not get
stuck if `trigger()` is already called.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun commented on code in PR #23564:
URL: https://github.com/apache/flink/pull/23564#discussion_r1369909942
##
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/OperationRelatedITCase.java:
##
@@ -119,18 +119,25 @@ List submitOperation() throws Exception {
SessionHandle sessionHandle = new
SessionHandle(UUID.fromString(sessionHandleId));
assertThat(SQL_GATEWAY_SERVICE_EXTENSION.getSessionManager().getSession(sessionHandle))
.isNotNull();
+
+OneShotLatch startLatch = new OneShotLatch();
+Thread main = Thread.currentThread();
OperationHandle operationHandle =
SQL_GATEWAY_SERVICE_EXTENSION
.getService()
.submitOperation(
sessionHandle,
() -> {
try {
-TimeUnit.SECONDS.sleep(10);
+startLatch.trigger();
+// keep operation in RUNNING state in
response to cancel
+// or close operations.
+main.join();
} catch (InterruptedException ignored) {
}
return NotReadyResult.INSTANCE;
});
+startLatch.await();
Review Comment:
Thanks @WencongLiu.
The Runnable work above is asynchronous, and the status will be turned to
RUNNING only when runBefore method
is executed. So we need a latch to prevent the state has not been turned
from PENDING into RUNNING yet.
https://github.com/apache/flink/blob/22c1eb44df7226c8e9045789e45c71c93668c644/flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/service/operation/OperationManager.java#L247-L269
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
WencongLiu commented on code in PR #23564:
URL: https://github.com/apache/flink/pull/23564#discussion_r1369895717
##
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/OperationRelatedITCase.java:
##
@@ -119,18 +119,25 @@ List submitOperation() throws Exception {
SessionHandle sessionHandle = new
SessionHandle(UUID.fromString(sessionHandleId));
assertThat(SQL_GATEWAY_SERVICE_EXTENSION.getSessionManager().getSession(sessionHandle))
.isNotNull();
+
+OneShotLatch startLatch = new OneShotLatch();
+Thread main = Thread.currentThread();
OperationHandle operationHandle =
SQL_GATEWAY_SERVICE_EXTENSION
.getService()
.submitOperation(
sessionHandle,
() -> {
try {
-TimeUnit.SECONDS.sleep(10);
+startLatch.trigger();
+// keep operation in RUNNING state in
response to cancel
+// or close operations.
+main.join();
} catch (InterruptedException ignored) {
}
return NotReadyResult.INSTANCE;
});
+startLatch.await();
Review Comment:
Why we need `startLatch` here? Consider this extreme case, the main thread
execute `startLatch.await();` after `startLatch.trigger();` is executed. Then
the main thread will get stuck here.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
WencongLiu commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1776862435 Sorry for the late reply. I'll take a look 😊. @Jiabao-Sun -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1776841973 Hi @xuyangzhong, could you help review this when you have time? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1775527798 Hi @XComp, could you help with it as well? Thanks :) -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
flinkbot commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1774393422 ## CI report: * bff9234113ad13e9d535bef3761b0a61a168f956 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun commented on PR #23564: URL: https://github.com/apache/flink/pull/23564#issuecomment-1774391325 Hi @WencongLiu, @fsk119. Could you help review it when you have time? Thanks. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis [flink]
Jiabao-Sun opened a new pull request, #23564: URL: https://github.com/apache/flink/pull/23564 ## What is the purpose of the change [FLINK-32661][sql-gateway] Fix unstable OperationRelatedITCase.testOperationRelatedApis ## Brief change log The operation execution is asynchronous and the status will only be updated from Pending to Running after calling from the runBefore method. ``` Jul 20 04:23:49 org.opentest4j.AssertionFailedError: Jul 20 04:23:49 Jul 20 04:23:49 Expecting actual's toString() to return: Jul 20 04:23:49 "PENDING" Jul 20 04:23:49 but was: Jul 20 04:23:49 "RUNNING" Jul 20 04:23:49 at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) Jul 20 04:23:49 at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) Jul 20 04:23:49 at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) Jul 20 04:23:49 at org.apache.flink.table.gateway.rest.OperationRelatedITCase.testOperationRelatedApis(OperationRelatedITCase.java:91) Jul 20 04:23:49 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) Jul 20 04:23:49 at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) Jul 20 04:23:49 at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) Jul 20 04:23:49 at java.lang.reflect.Method.invoke(Method.java:498) Jul 20 04:23:49 at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131) Jul 20 04:23:49 at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156) Jul 20 04:23:49 at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147) Jul 20 04:23:49 at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92) Jul 20 04:23:49 at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86) Jul 20 04:23:49 at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:21 ``` ## Verifying this change This change is already covered by existing tests. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not documented) -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
