You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/07/31 20:24:38 UTC
[GitHub] [helix] pkuwm opened a new pull request #1199: Remove duplicate classes in different jars
pkuwm opened a new pull request #1199:
URL: https://github.com/apache/helix/pull/1199
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1198
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
There are duplicate classes at runtime since 1.0+, because of the module separation. Two different JARs contain the same package but with different contents. Eg, both helix-core.jar and metrics-common.jar contains SensorNameProvider.class. And at tests runtime, we have a check to detect duplicate classes and this causes tests failure.
Though helix doesn't have duplicate classes in the same package, it is still not a good practice to package the same class into two different jars that have dependency relation.
It is necessary to clean up the duplicate classes. And small refactoring work is needed to fix this issue.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Copy & paste the result of "mvn test")
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r464211705
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
Thanks for pasting the log message, but this doesn't seem like it's coming from Maven. Is this coming from some other build tool like gradle? Since this detail is missing in the issue/PR description, could you please add this detail?
I think this could be solved by tweaking the ivy file like you did here - do you think we could try to resolve the conflict without changing package names/moving packages?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1199:
URL: https://github.com/apache/helix/pull/1199#issuecomment-667600349
> > > The movement of the classes is logically not what we intended to do. We do want them in the common package. Let's think about better way to do it. But glad to know there are not many classes impacted.
>
> >
>
> > @jiajunwang
>
> > For current structure, helix-common is actually redundant as only helix-core depends on helix-common. And helix-common has the same package paths with helix-core. That being said, it is totally fine to just merge helix-common to helix-core. With current structure, I don't see a value that we have helix-common but only adds redundancy.
>
> >
>
>
>
>
>
> It was an explicit design choice when we separated helix-common from helix-core. We shouldn't consider merging it back into helix-core as that would further complicate the module structure.
Then could you give a better proposal solution for the issue?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463897625
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
Thanks for pointing it out. I think this does remove classes from helix jars: helix-core.jar. Files are not removed from code base, but from the exported jars. Anyway, changed removed to resolved.
The classes in metrics-common have the same package path org.apache.helix.monitoring as those classes in helix-core. The challenge is, with existing bundle plugin/strategy in helix, there is no way to exclude the metric-common's classes in helix-core.jar.
Even we could change the bundle plugin to support class filtering, I still don't think it is clean if we still put same package classes in different jars.
The right way for different modules/jars should have different package paths, eg.:
metrics-common: org.apache.helix.monitoring.common
helix-core: org.apache.helix.monitoring
With these different package paths, we could cleanly avoid exporting classes of metrics-common to helix-core.jar.
This is the reason why the PR would like to differentiate the package paths for different modules/jars.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463979391
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
So what you're claiming here is when maven packages all of them into one helix-core.jar, that jar ends up having multiple copies of the same class with the same package name?
The description in the issue https://github.com/apache/helix/issues/1198 is still not too clear - could you explain what the failure is exactly?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463906943
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
I guess you missed something about the problem. There are no conflicting classes in helix-core. All classes are unique/only in helix codebase. The problem is, when we package the helix jars, the dependent subproject's classes are packaged into helix-core.jar: all classes in module helix-core and its dependencies match `org.apache.helix.*` are packaged to helix-core.jar. Then there are duplicate classes from metrics-common.jar/zookeeper-api.jar/helix-core.jar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463906344
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
Why don't we just remove the conflicting classes from helix-core? That is what my 2nd paragraph is saying. If you just change the package structure for the conflicting classes in metrics-common, you'd be masking the problem (the duality still exists) instead of solving the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Remove duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463888755
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
Thanks for identifying the issue and proposing a potential solution. However, I think the title of this PR/commit is a bit misleading - are we removing any classes? I'm not seeing any files removed in this PR..
I think instead of moving dual-existent classes so that they have different import paths, we should identify which classes both exist in both metrics-common and helix-core, and just remove the ones in helix-core. They were initially left in helix-core, but we should remove them clean since they're causing issues. This way, we won't have to modify any package names and the issue we get from duplicate classes at runtime will no longer be there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Remove duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463888755
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
I think the title of this PR/commit is a bit misleading - are we removing any classes?
I think instead of moving dual-existent classes so that they have different import paths, we should identify which classes both exist in both metrics-common and helix-core, and just remove the ones in helix-core. They were initially left in helix-core, but we should remove them clean since they're causing issues. This way, we won't have to modify any package names and the issue we get from duplicate classes at runtime will no longer be there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463897625
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
Thanks for pointing out the misleading title. Changed it to resolved.
The classes in metrics-common have the same package path org.apache.helix.monitoring as those classes in helix-core. The challenge is, with existing bundle plugin/strategy in helix, there is no way to exclude the metric-common's classes in helix-core.jar.
Even we could change the bundle plugin to support class filtering, I still don't think it is clean if we still put same package classes in different jars.
The right way for different modules/jars should have different package paths, eg.:
metrics-common: org.apache.helix.monitoring.common
helix-core: org.apache.helix.monitoring
With these different package paths, we could cleanly avoid exporting classes of metrics-common to helix-core.jar.
This is the reason why the PR would like to differentiate the package paths for different modules/jars.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r464212579
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
Also, this part was a little misleading:
> And at tests runtime, we have a check to detect duplicate classes and this causes tests failure.
I wanted to check if this test is referring to some test happening outside of the Helix maven project? Could you confirm?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1199:
URL: https://github.com/apache/helix/pull/1199#issuecomment-667557388
> > The movement of the classes is logically not what we intended to do. We do want them in the common package. Let's think about better way to do it. But glad to know there are not many classes impacted.
>
> @jiajunwang
> For current structure, helix-common is actually redundant as only helix-core depends on helix-common. And helix-common has the same package paths with helix-core. That being said, it is totally fine to just merge helix-common to helix-core. With current structure, I don't see a value that we have helix-common but only adds redundancy.
>
It was an explicit design choice when we separated helix-common from helix-core. We shouldn't consider merging it back into helix-core as that would further complicate the module structure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463906943
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
I guess you missed something about the problem. There are no conflicting classes in helix-core. All classes are unique/only in helix codebase. The problem is, when we package the helix jars, the dependent subproject's classes are packaged into helix-core.jar: all classes in module helix-core and its dependencies that match `org.apache.helix.*` are all packaged to helix-core.jar. Then there are duplicate classes from metrics-common.jar/zookeeper-api.jar/helix-core.jar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm closed pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm closed pull request #1199:
URL: https://github.com/apache/helix/pull/1199
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463986825
##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterEventMonitor.java
##########
@@ -26,10 +26,10 @@
import com.codahale.metrics.Histogram;
import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.DynamicMBeanProvider;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.DynamicMetric;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.HistogramDynamicMetric;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.SimpleDynamicMetric;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.DynamicMBeanProvider;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.DynamicMetric;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.HistogramDynamicMetric;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.SimpleDynamicMetric;
Review comment:
When users import helix-core, since helix-core depends on helix-common, helix-common.jar and its transitive dependencies are also imported.
I understand the purpose of not changing packaging naming, but packaging dependencies into helix-core is not well handled, which causes the duplicate class issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463986084
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
> The problem is, when we package the helix jars, the dependent subproject's classes are packaged into helix-core.jar: all classes in module helix-core and its dependencies match org.apache.helix.* are packaged to helix-core.jar. Then there are duplicate classes from metrics-common.jar/zookeeper-api.jar/helix-core.jar
I believe this is clear. And also I've pasted the failure log message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463978956
##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ClusterEventMonitor.java
##########
@@ -26,10 +26,10 @@
import com.codahale.metrics.Histogram;
import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.DynamicMBeanProvider;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.DynamicMetric;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.HistogramDynamicMetric;
-import org.apache.helix.monitoring.mbeans.dynamicMBeans.SimpleDynamicMetric;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.DynamicMBeanProvider;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.DynamicMetric;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.HistogramDynamicMetric;
+import org.apache.helix.monitoring.common.mbeans.dynamicMBeans.SimpleDynamicMetric;
Review comment:
The original package naming was intentional in order to not cause existing applications to have to change import statements (code change). Why is this needed? Are users importing multiple modules at the same time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1199:
URL: https://github.com/apache/helix/pull/1199#issuecomment-667440493
> The movement of the classes is logically not what we intended to do. We do want them in the common package. Let's think about better way to do it. But glad to know there are not many classes impacted.
@jiajunwang
For current structure, helix-common is actually redundant as only helix-core depends on helix-common. And helix-common has the same package paths with helix-core. That being said, it is totally fine to just merge helix-common to helix-core. With current structure, I don't see a value that we have helix-common but only adds redundancy.
The only movement of classes is in metrics-common. The purpose is to clearly give a dedicated package path for metrics-common and cleanly avoid exporting classes of metrics-common to helix-core.jar.
I 100% agree that we should bring up a good plan to clean up the structure. This is what I would like to push forward to make helix code structure clear. And I believe you do, as well, and you possibly have an idea to restructure code base. :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1199: Resolve duplicate classes in different helix jars
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1199:
URL: https://github.com/apache/helix/pull/1199#discussion_r463986084
##########
File path: helix-core/src/main/java/org/apache/helix/InstanceType.java
##########
@@ -22,7 +22,7 @@
import java.util.Arrays;
import java.util.List;
-import org.apache.helix.monitoring.mbeans.MonitorDomainNames;
+import org.apache.helix.monitoring.common.mbeans.MonitorDomainNames;
Review comment:
> The problem is, when we package the helix jars, the dependent subproject's classes are packaged into helix-core.jar: all classes in module helix-core and its dependencies match org.apache.helix.* are packaged to helix-core.jar. Then there are duplicate classes from metrics-common.jar/zookeeper-api.jar/helix-core.jar
I believe this is clear. And also I've pasted the failure log message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org