[FLINK-10964] SQL-client throws exception when paging through finished batch query #7265

yanghua · 2018-12-09T12:31:16Z

What is the purpose of the change

This pull request fixes SQL-client throws exception when paging through finished batch query

Brief change log

Fix SQL-client throws exception when paging through finished batch query

Verifying this change

This change added tests and can be verified as follows:

testRetrieveBoundedStreamQueryExecutionChangelog()
testRetrieveBoundedStreamQueryExecutionTable()
testRetrieveResultAfterBatchQueryFinished

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

twalthr

Thank you @yanghua. I found a little bug and added some comments regarding proper test separation.

twalthr · 2019-01-07T10:56:37Z

...flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliChangelogResultView.java

 			// stop retrieval if job is done
 			case EOS:
-				stopRetrieval();
+				stopRetrieval(TypedResult.ResultType.EOS);


Use a boolean flag cleanUpQuery instead.

twalthr · 2019-01-07T11:02:35Z

...ibraries/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliResultView.java

 		public volatile boolean isRunning = true;

+		//init with running status
+		public volatile TypedResult.ResultType jobStatus = TypedResult.ResultType.PAYLOAD;


Call this cleanUpQuery and make it a boolean.

twalthr · 2019-01-07T11:07:36Z

...ibraries/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliResultView.java

+	protected void stopRetrieval(TypedResult.ResultType jobStatus) {
 		// stop retrieval
 		refreshThread.isRunning = false;
+		refreshThread.jobStatus = jobStatus;


Switch these two statements because otherwise the refresh thread can already do side effects when isRunning is set to false.

twalthr · 2019-01-07T11:10:20Z

...ql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java

+
+	private static final class TestingCliResultView implements Runnable {
+
+		private final CliResultView resultView;


This test is meant for testing the LocalExecutor. Please don't use CLI classes here for separation of concerns. The tests should only perform calls to the executor interface.

To verify you changes feel free to create a new CliResultViewTest or something like that. The test coverage is still little for the SQL Client and needs to be improved.

twalthr · 2019-01-07T11:11:44Z

...ql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java

+					cliChangelogResultViewRunner.start();
+
+					//wait enough time to trigger CliTableResultView#refresh call stopRetrieval method
+					Thread.sleep(5000);


Remove all hard sleeps because they make our tests slow.

yanghua · 2019-01-08T07:33:40Z

Travis CI failed due to FLINK-11280. cc @twalthr

twalthr

@yanghua I added another set of comments. Thanks.

twalthr · 2019-01-14T13:37:20Z

...ibraries/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliResultView.java

+	protected void stopRetrieval(boolean cleanUpQuery) {
 		// stop retrieval
-		refreshThread.isRunning = false;
+		if (refreshThread.isRunning) {


This change does still not fix the potential concurrency issue that I mentioned. Once isRunning is set to false the other thread might check for the cleanUpQuery value before the next line in this thread is evaluated. A proper implementation looks like:

refreshThread.cleanUpQuery = cleanUpQuery; refreshThread.isRunning = false;

twalthr · 2019-01-14T13:41:07Z

...ries/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliResultViewTest.java

+/**
+ * Contains basic tests for the {@link CliResultView}.
+ */
+public class CliResultViewTest {


This test still uses a full integration test just to verify if a couple of calls are forwarded to the executor. Instead of setting up an entire Flink cluster for it, we should simply create a mocking executor as it is done in org.apache.flink.table.client.cli.CliClientTest and verify the calls that CliResultView performs on it.

yanghua · 2019-01-15T15:48:29Z

@twalthr I have updated this PR.

…d batch query

twalthr · 2019-02-19T11:21:46Z

Thank you @yanghua. I will take care of merging this...

…ough finished batch query This closes apache#7265.

…ough finished batch query This closes #7265.

yanghua force-pushed the FLINK-10964 branch 2 times, most recently from facbf46 to f92058a Compare December 9, 2018 13:14

twalthr requested changes Jan 7, 2019

View reviewed changes

yanghua force-pushed the FLINK-10964 branch 2 times, most recently from 340128a to 51fa4b1 Compare January 7, 2019 12:49

yanghua force-pushed the FLINK-10964 branch from 46dd31d to e24c787 Compare January 10, 2019 09:47

twalthr requested changes Jan 14, 2019

View reviewed changes

yanghua force-pushed the FLINK-10964 branch from e24c787 to 58e76c9 Compare January 15, 2019 11:19

[FLINK-10964] SQL-client throws exception when paging through finishe…

57a5109

…d batch query

yanghua force-pushed the FLINK-10964 branch from 58e76c9 to 57a5109 Compare February 10, 2019 12:50

twalthr pushed a commit to twalthr/flink that referenced this pull request Feb 19, 2019

[FLINK-10964][sql-client] SQL Client throws exception when paging thr…

595a68a

…ough finished batch query This closes apache#7265.

asfgit closed this in adc5eef Feb 19, 2019

asfgit pushed a commit that referenced this pull request Feb 19, 2019

[FLINK-10964][sql-client] SQL Client throws exception when paging thr…

bf23cc4

…ough finished batch query This closes #7265.

asfgit pushed a commit that referenced this pull request Feb 19, 2019

[FLINK-10964][sql-client] SQL Client throws exception when paging thr…

9663323

…ough finished batch query This closes #7265.

rmetzger added the component=SQL/Client label Mar 18, 2019

flinkbot added component=TableSQL/Client and removed component=SQL/Client labels Mar 17, 2022


		private static final class TestingCliResultView implements Runnable {

		private final CliResultView resultView;

[FLINK-10964] SQL-client throws exception when paging through finished batch query #7265

[FLINK-10964] SQL-client throws exception when paging through finished batch query #7265

Uh oh!

Conversation

yanghua commented Dec 9, 2018

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

yanghua commented Jan 8, 2019

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

yanghua commented Jan 15, 2019

Uh oh!

twalthr commented Feb 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants