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

Feature#2601 Check RoutingResult. #3021

Merged
merged 5 commits into from
Sep 17, 2019
Merged

Conversation

sunbufu
Copy link
Contributor

@sunbufu sunbufu commented Sep 11, 2019

Fixes #2601.

Changes proposed in this pull request:

  1. add checker in ParsingSQLRouter for check RoutingResult.
  2. add ParsingSQLRoutingResultChecker to check RoutingResult with ShardingRule.RoutingUnits.
  3. add, modify some UTs.

2. add ParsingSQLRoutingResultChecker to check RoutingResult with ShardingRule.RoutingUnits.
3. add, modify some UTs.
Copy link
Member

@tuohai666 tuohai666 left a comment

Choose a reason for hiding this comment

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

I notice you use reflect. Have you tested the performance?
Is the checking work for each SQL?

@sunbufu
Copy link
Contributor Author

sunbufu commented Sep 11, 2019

Reflect is only in AbstractSpringJUnitTest.java for unit test to reload MetaData after create table.

@sunbufu
Copy link
Contributor Author

sunbufu commented Sep 11, 2019

We can discuss which sql should be check. In my view, we can divide the check into two parts, dataSource check and table check. In this PR table check for DML, and dataSource check for no master-slaver.

@tuohai666
Copy link
Member

It's right to check all the DMLs. You can take a performance test for comparision.

@sunbufu
Copy link
Contributor Author

sunbufu commented Sep 12, 2019

Hello, I don't think the checker will affect performance. Because it only check routingResult with shardingRule and meteData. and I don't know how to prove it. Please give me some advice.

@tuohai666
Copy link
Member

You can compare the performance (QPS) between checking and without checking routingResult.
This work can be done by a performance tool like jmeter.

@tuohai666
Copy link
Member

The ckecker do affect the performance.
One query stay only several microsecond in ShardingSphere. Any additional operation, especially String operation (There's String + String in your codes), will affect the performance.

@sunbufu
Copy link
Contributor Author

sunbufu commented Sep 12, 2019

OK, I will do this after mid-autumn festival.

@coveralls
Copy link

coveralls commented Sep 16, 2019

Pull Request Test Coverage Report for Build 359

  • 83 of 87 (95.4%) changed or added relevant lines in 4 files are covered.
  • 147 unchanged lines in 35 files lost coverage.
  • Overall coverage increased (+0.4%) to 64.348%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sharding-core/sharding-core-route/src/main/java/org/apache/shardingsphere/core/route/router/sharding/ParsingSQLRoutingResultChecker.java 78 82 95.12%
Files with Coverage Reduction New Missed Lines %
sharding-core/sharding-core-optimize/src/main/java/org/apache/shardingsphere/core/optimize/encrypt/engine/dml/EncryptInsertOptimizeEngine.java 1 91.67%
sharding-core/sharding-core-optimize/src/main/java/org/apache/shardingsphere/core/optimize/encrypt/statement/EncryptInsertOptimizedStatement.java 1 85.71%
sharding-core/sharding-core-optimize/src/main/java/org/apache/shardingsphere/core/optimize/sharding/engnie/dml/ShardingUpdateOptimizeEngine.java 1 90.0%
sharding-core/sharding-core-optimize/src/main/java/org/apache/shardingsphere/core/optimize/sharding/segment/condition/ShardingCondition.java 1 75.0%
sharding-core/sharding-core-optimize/src/main/java/org/apache/shardingsphere/core/optimize/sharding/statement/dml/ShardingConditionOptimizedStatement.java 1 83.33%
sharding-core/sharding-core-parse/sharding-core-parse-common/src/main/java/org/apache/shardingsphere/core/parse/sql/statement/dml/InsertStatement.java 1 96.97%
sharding-core/sharding-core-rewrite/src/main/java/org/apache/shardingsphere/core/rewrite/token/generator/InsertCipherNameTokenGenerator.java 1 93.75%
sharding-core/sharding-core-rewrite/src/main/java/org/apache/shardingsphere/core/rewrite/token/generator/InsertSetQueryAndPlainColumnsTokenGenerator.java 1 97.06%
sharding-core/sharding-core-rewrite/src/main/java/org/apache/shardingsphere/core/rewrite/token/generator/SelectItemsTokenGenerator.java 1 95.0%
sharding-core/sharding-core-rewrite/src/main/java/org/apache/shardingsphere/core/rewrite/token/generator/UpdateEncryptColumnTokenGenerator.java 1 96.88%
Totals Coverage Status
Change from base Build 349: 0.4%
Covered Lines: 9481
Relevant Lines: 14734

💛 - Coveralls

@sunbufu
Copy link
Contributor Author

sunbufu commented Sep 16, 2019

I run jmeter with SELECT * FROM t_order WHERE order_id = 3 (2 threads, 100 loop counts) and find there was not performance impact basically. Because there is no String + String operation when check pass, and JVM will optimize String + String operation.

@tuohai666
Copy link
Member

I run jmeter with SELECT * FROM t_order WHERE order_id = 3 (2 threads, 100 loop counts) and find there was not performance impact basically. Because there is no String + String operation when check pass, and JVM will optimize String + String operation.

OK.
Please resolve the coverage decreasing.

@tuohai666 tuohai666 self-requested a review September 17, 2019 03:42
@terrymanu terrymanu merged commit adc33c4 into apache:dev Sep 17, 2019
@terrymanu terrymanu added this to the 4.0.0-RC3 milestone Sep 17, 2019
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.

Verify the shardingRule during the routing procedure
4 participants