-
Notifications
You must be signed in to change notification settings - Fork 52
HBASE-29225 Add module for Jetty 12 with EE8 to hbase-thirdparty #131
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| */ | ||
| ON MVN COMPILE NOT WORKING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to copy this header? this file is copy of original jetty module
|
I am not sure about the name, but jetty12 wouldn't be great either. |
Yes hence thought to create env specific names.
Yes this is a fair concern. Although, we would not want to have shaded jetty9 in our classpath given we rely on shaded jetty12-eeX. But I am afraid what if hadoop timeline service or something else brings it in indirectly.? |
This comment was marked as outdated.
This comment was marked as outdated.
IUC since the eeX stuff is in separate packages, we could add any or all ee specification modules. As bad a jetty12 would be a bad name, at least we're not tying it to the ee spec version. Maybe jetty-new ? or Jetty-b ?
Ideally, thirdparty netty shouldn't be brought in by the hbase client. ( I have not checked if that's true) |
Exactly that is the plan. Please refer https://issues.apache.org/jira/browse/HBASE-29224
|
I am not sure if I understand you correctly but we may need all versions of jetty to exist:
Hence created a new module for ee8 as first step. |
AFAICT ee8 and ee9 does not have to be a separate project. So we could maintain a single hbase-shaded-netty-whatever package, that includes ee8 for now, and we can add ee9 later (while retaining ee8 for older hbase branches). |
Okay got your point. Agreed, created 2 modules as we have a javax dependency inside thirdparty jetty (not sure why), copied same inside new module. If we can build without this bundled, then we should be good maybe. |
Updated PR with both EE8 + EE9 changes (for testing purpose), will rerun UTs locally and update. Also renamed hbase-shaded-jetty-ee8 to hbase-shaded-jetty-12-plus for time being |
This comment was marked as outdated.
This comment was marked as outdated.
|
Do you think it would make sense to split this up, so that we have a base shaded jetty package, and separate jee8 and jee9 shaded packages ? |
I ran the UTs with the apache/hbase#6783 and all tests passed locally. We are good with a single package for eeX. So javax dependency was indeed redundant. Should I retain the ee9 change here or revert? WDYT?
In HBase it may not help as our jetty-core and eeX specific imports happen together. I mean we don't seem to have an module which only depends on jetty but not eeX. We would eventually end up with including both dependencies to our module pom's. |
|
My use case would be for example:
Not a huge deal either way, but saves a bit on the assembly size. |
Ah yes we could definitely save a few MBs here and avoid bundling redundant code. Just to be clear, we are suggesting to break into modules as below:
So basically fall back to the initial approach of this PR, but maybe a new module for base? Right? |
Right |
Cool sounds good to me, should I update the PR with the suggestion? or wait sometime for what others opine? I am good either ways, simple change. |
|
If you agree, then I think updating it now would let the others see what our current best approach is. |
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Will go ahead and merge this today.? could not get any more reviews here. |
|
Bumped to jetty 12.0.20, pending CI |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
… otherwise getting error when including core into sub module
Bump to latest jetty
… - Discrepancy between Jetty 9 and Jetty 12 when setting the base resource to a path containing ..
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…rty (apache#131)" This reverts commit 6016e75.
…che#131) This change adds a new set of modules which we will need for Jetty 12 migration. The code has been modularised into following modules: - `hbase-shaded-jetty-12-plus-core`: Contains shaded jetty 12 core jars - `hbase-shaded-jetty-12-plus-ee8`: Contains shaded jetty EE8 specific jars So basically: - Branches which want to consume EE8 may need to add `hbase-shaded-jetty-12-plus-core` and `hbase-shaded-jetty-12-plus-ee8` in their dependency replacing the former `hbase-shaded-jetty` - Branches which want to consume EE9/EE10 may need to add a new module for same in `hbase-thirdparty` in future Signed-off-by: Istvan Toth <stoty@apache.org>
This PR adds a new set of modules which we will need for Jetty 12 migration. The code has been modularised into 3 modules:
hbase-shaded-jetty-12-plus-core: Contains shaded jetty 12 core jarshbase-shaded-jetty-12-plus-ee8: Contains shaded jetty EE8 specific jarsSo basically:
hbase-shaded-jetty-12-plus-coreandhbase-shaded-jetty-12-plus-ee8in their dependency replacing the formerhbase-shaded-jettyhbase-thirdpartyin future