You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/03/13 13:44:44 UTC
[GitHub] [shardingsphere] sandynz opened a new pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
sandynz opened a new pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036
Changes proposed in this pull request:
- Add Pair impl and remove commons-lang3 dependency in pipeline modules. But there's still implicit dependency of `commons-lang3`.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036#issuecomment-1070884202
I missed `LazyInitializer` before, it's used in pipeline modules.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz closed pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
sandynz closed pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036#issuecomment-1072983239
Closed now, it need more investigation on commons-lang3 dependency.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] terrymanu commented on pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
terrymanu commented on pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036#issuecomment-1067591040
How about split the pair as updated map and new collection?
If the key is null, it means there are not original config, which should belong to new collection.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036#issuecomment-1066443688
> How about use Map to instead of Pair?
I've investigated on this topic. It's possible to use Map instead of Pair, but it might has some side effects. Details as following.
Since there's
```
for (Entry<Class<? extends YamlRuleConfiguration>, YamlRuleConfiguration> entry : targetRulesMap.entrySet()) {
if (!sourceRulesMap.containsKey(entry.getKey())) {
result.add(Pair.of(null, entry.getValue()));
}
}
```
There might be several key elements are `null`, so we need to put `null` as value and use value elements as map key else some value elements will be replaced, e.g. `map.put(entry.getValue(), null)`.
Considering the scaling scenario, the possible side effects if we change `Collection<Pair>` to `Map`:
1. `createJobConfig` just need a collection or array to iterate, no `get`, it doesn't really need hash or tree map. According to `Development Guidelines` readability, using `Collection<Pair>` is better for code purpose than using `Map`.
2. For `boolean isRuleAltered(YamlRuleConfiguration sourceRuleConfig, YamlRuleConfiguration targetRuleConfig)` method, the first and second parameter's meaning is defined.
- 1) When using `Map`, target rule config might be transferred to `sourceRuleConfig`, which is confusing.
- 2) Currently, `isRuleAltered` method just do rough comparison of old version rule and new version rule, but we need to do detailed comparison later, some rule change doesn't mean scaling job is necessary. Transfer target rule config to `sourceRuleConfig` might cause issue, e.g. if dataSources is not changed, cleanup sharding rule might not need to do scaling, but add sharding rule need to do scaling. So when we exchange `sourceRuleConfig` and `targetRuleConfig`, they're not equivalent.
- 3) It's possible to keep parameters ordering when invoking `isRuleAltered` method, but it make code more complicated.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036#issuecomment-1067672513
> How about split the pair as updated map and new collection? If the key is null, it means there are not original config, which should belong to new collection.
My investigation:
It's possible to do it, but the code might be more complicated than before.
Current code snippet:
```
private Optional<JobConfiguration> createJobConfig(final StartScalingEvent event) {
//...
for (Pair<YamlRuleConfiguration, YamlRuleConfiguration> each : groupSourceTargetRuleConfigsByType(sourceRootConfig.getRules(), targetRootConfig.getRules())) {
//...
}
private Collection<Pair<YamlRuleConfiguration, YamlRuleConfiguration>> groupSourceTargetRuleConfigsByType(final Collection<YamlRuleConfiguration> sourceRules,
final Collection<YamlRuleConfiguration> targetRules) {
//...
}
```
Currently, `groupSourceTargetRuleConfigsByType` method return unified `Collection<Pair>`, and `createJobConfig` method just need one `for` loop.
If we split pairs to updated map and new collection, possible changes:
1. Split `groupSourceTargetRuleConfigsByType` method to `2` methods, one return `Map` and another return `Collection<ruleConfig>`.
2. In `createJobConfig`, add another `for` loop to iterate `Collection<ruleConfig>`.
3. In `createJobConfig`, in order to reuse the code of `for` loop, we need to extract it into a new method.
It might be not worth to do it.
Another consideration, pair and tuple are basic data structrue in some programming languages, they're useful sometimes.
So I think it's better to keep pair in project when it's needed.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #16036: Add Pair impl and remove commons-lang3 dependency in pipeline modules
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #16036:
URL: https://github.com/apache/shardingsphere/pull/16036#issuecomment-1070884202
I missed `LazyInitializer` before, it's used in pipeline modules.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org