[GitHub] ctubbsii commented on a change in pull request #995: fixes #980
ctubbsii commented on a change in pull request #995: fixes #980 URL: https://github.com/apache/fluo/pull/995#discussion_r165750259 ## File path: modules/core/pom.xml ## @@ -73,6 +73,10 @@ org.apache.hadoop hadoop-client + + org.apache.thrift + libthrift Review comment: It changed a few things... interesting things... but not important things. The reduced POM looks like our POM, so that's good. I'll check the jar contents manually just to do a quick sanity check, but so far, everything looks the way I'd expect. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #995: fixes #980
ctubbsii commented on a change in pull request #995: fixes #980 URL: https://github.com/apache/fluo/pull/995#discussion_r162852338 ## File path: modules/core/pom.xml ## @@ -73,6 +73,10 @@ org.apache.hadoop hadoop-client + + org.apache.thrift + libthrift Review comment: This works, and it probably avoids us having to do anything with the dependency reduced POM... although I would do a comparison with the dependency reduced version manually... just to make sure nothing strange was overlooked. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #995: fixes #980
ctubbsii commented on a change in pull request #995: fixes #980 URL: https://github.com/apache/fluo/pull/995#discussion_r162149226 ## File path: modules/core/pom.xml ## @@ -141,6 +132,34 @@ + +org.apache.maven.plugins +maven-shade-plugin + + + + shade + +package + + + + org.apache.thrift + org.shaded.thrfit + + + + + org.apache.fluo.core.* + org.shaded.thrift:* + org.apache.thrift:* + + + false Review comment: If this POM is created, I wonder if it would replace the original POM when installed to the local maven repository or deployed to the remote maven repository. If so, it may be useful to let the plugin create this. See comment above about libthrift dependency being resolved transitively. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #995: fixes #980
ctubbsii commented on a change in pull request #995: fixes #980 URL: https://github.com/apache/fluo/pull/995#discussion_r162148019 ## File path: modules/core/pom.xml ## @@ -141,6 +132,34 @@ + +org.apache.maven.plugins +maven-shade-plugin + + + + shade + +package + + + + org.apache.thrift + org.shaded.thrfit Review comment: Typo here... but also, the new package should be a subpackage of `org.apache.fluo.core` (or whatever the base package is for this fluo module). This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #995: fixes #980
ctubbsii commented on a change in pull request #995: fixes #980 URL: https://github.com/apache/fluo/pull/995#discussion_r162147758 ## File path: modules/core/pom.xml ## @@ -73,6 +73,10 @@ org.apache.hadoop hadoop-client + + org.apache.thrift + libthrift Review comment: I think this still needs to be `provided`, or perhaps `true` in order to prevent transitive dependency resolution by consumers of the fluo-core jar. That is, unless the shade plugin also modifies the POM to exclude libthrift... and I'm not sure it does. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #995: fixes #980
ctubbsii commented on a change in pull request #995: fixes #980 URL: https://github.com/apache/fluo/pull/995#discussion_r162148726 ## File path: modules/core/pom.xml ## @@ -141,6 +132,34 @@ + +org.apache.maven.plugins +maven-shade-plugin + + + + shade + +package + + + + org.apache.thrift + org.shaded.thrfit + + + + + org.apache.fluo.core.* + org.shaded.thrift:* + org.apache.thrift:* Review comment: I'm not sure these includes are correct. The first one looks like a package pattern. The other two look like maven artifacts, but only the groupId. But, there's no such thing as a groupId named `org.shaded.thrift`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
