[FLINK-34566] Pass a FixedThreadPool to set reconciliation parallelism correctly #790
Conversation
| for (int i = 0; i < testParallelism * 2; i++) { | ||
| threadPoolExecutor.execute( | ||
| () -> { | ||
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
what do we expect to test here?
There was a problem hiding this comment.
I want to test whether the number of threads in the executor can reach our expected maximumPoolSize when the executor receives enough tasks.
see assertion in line 87
There was a problem hiding this comment.
actually, I think code in lines 78 to 85 was ugly, any good suggestion?
There was a problem hiding this comment.
Maybe we could do a more functional test that will hold even if the service implementation changes.
- Set a smaller parallelism to not use soo many threads
- For 2x the parallelism like you did we execute:
- Increment atomic counter
- Sleep for long (lets say 10sec)
- Assert the counter == 2x parallelism
Wdyt? It would be good to test that the test actually fails before the change
There was a problem hiding this comment.
do you mean "increase counter and sleep for long in every task" ? if yes, "Assert the counter == 2x parallelism" will always fail even if we set a fixed thread pool(what we want to change)
I think check pool size (which equals really thread number）is functional enough.
What is the purpose of the change
This pull request pass a fixed size thread pool to JOSDK, so that we can set a large reconciliation parallelism as we except .
Brief change log
Verifying this change
This change added tests and can be verified as follows:
org.apache.flink.kubernetes.operator.FlinkOperatorTest#testConfigurationPassedToJOSDK:
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors: ( no)Documentation