Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SingleSharding process keep pace with MultipleSharding process #1911

Closed
wants to merge 2 commits into from

Conversation

skaic
Copy link
Contributor

@skaic skaic commented Jun 28, 2021

Fixes #1910.

Changes proposed in this pull request:

  • remove SingleSharding process run in currentThread
  • catch ExecutionException and throw the JobSystemException of executorService.submit
  • assertExecuteFailureWhenThrowExceptionForMultipleShardingItems test is JobSystemException

@codecov-commenter
Copy link

Codecov Report

Merging #1911 (7c543e4) into master (adcf88c) will decrease coverage by 0.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1911      +/-   ##
============================================
- Coverage     85.66%   85.57%   -0.09%     
+ Complexity      114      113       -1     
============================================
  Files           276      276              
  Lines          6026     6031       +5     
  Branches        922      922              
============================================
- Hits           5162     5161       -1     
- Misses          525      529       +4     
- Partials        339      341       +2     
Impacted Files Coverage Δ
...sphere/elasticjob/executor/ElasticJobExecutor.java 89.69% <83.33%> (-1.62%) ⬇️
...e/shardingsphere/elasticjob/infra/env/IpUtils.java 60.00% <0.00%> (-4.62%) ⬇️
...sticjob/reg/zookeeper/ZookeeperRegistryCenter.java 72.44% <0.00%> (-0.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adcf88c...7c543e4. Read the comment docs.

Copy link
Member

@TeslaCN TeslaCN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Future and CountDownLatch are doing the same thing, aren't they?
My understanding is submitting a task to the thread pool may cause a thread context switch, which is unnecessary for a single shard.
I think it is better to keep it as it unless we have good reasons.

@skaic
Copy link
Contributor Author

skaic commented Jun 29, 2021

Thanks for the reply. @TeslaCN

I'm trying work of #1464 .
So I need all process run are same currentThread or executorService.submit at first.

@TeslaCN
Copy link
Member

TeslaCN commented Jun 29, 2021

Thanks for the reply. @TeslaCN

I'm trying work of #1464 .
So I need all process run are same currentThread or executorService.submit at first.

I think it is better to discuss about how before doing that.

@skaic
Copy link
Contributor Author

skaic commented Jun 29, 2021

Yes, you are right.

@skaic skaic closed this Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ElasticJobExecutor execute have some different between SingleSharding and MultipleSharding
3 participants