HBASE-26546: hbase-shaded-client missing required thirdparty classes under hadoop 3.3.1 #3926
Conversation
…under hadoop 3.3.1
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
I did some investigation and cannot figure out why this works without tweaking our allow list. In fact, I checked out the original state prior to #3184 and (after applying #3299, which adds a different necessary exclusion) it also succeeds there. For reference, I'm running the following locally to test (which confirms runs and succeeds invariant checks):
I've also manually verified that the jars look correct: I also diffed the Finally, I also pushed this to our internal fork and verified that my end-client which was previously failing is now succeeding. It seems like this should be good to merge as-is. @saintstack since you originally ran into this failure, what do you think? |
|
I just tried this on branch-2:
... which is probably how I produced the fail originally but it works w/ this patch applied. I can't explain why it works now (context is long gone). Thanks for digging in. Try it. +1. |
|
Is this something we can verify in our nightly build test suite? We already have a stage that goes out of its way to run the client against the pseudo-distributed cluster, maybe this could be extended to use the shaded client? https://github.com/apache/hbase/blob/master/dev-support/Jenkinsfile#L752 |
|
I see. This failure might indeed be detected by our current test case, if but for the fact that we set |
|
@ndimiduk is there something you'd like me to add to this PR? Sorry, I'm not super familiar with our test suite configurations. I could naively change that HADOOP3 version to 3.3.1, but not sure what the effect would be or if there is a more thorough approach to take? |
|
I took a closer look at the files you referenced. I actually think we wouldn't have caught this, even if the hadoop version were correct. As far as I can tell, Would making that change actually show up in these PR checks or only as part of some nightly process? Should we also bump the HADOOP3_VERSION, or run against both 3.1.1/3.3.1? |
|
I think @busbey is most knowledgable about this specific check, so I'll let him weigh in. I would say that if we have a client-consumable artifact that is not currently covered by this check, we should add it. If the upstream Hadoop releases are materially different, we should extend the test coverage to run the full supported matrix, as described in https://hbase.apache.org/book.html#hadoop The largest obstacles to this level of coverage are test resource capacity in general and time-to-completion specifically. |
|
For an example of the lengths we go to in order to assert basic compile-time compatibility with myriad Hadoop releases during pre-commit, see https://github.com/apache/hbase/blob/master/dev-support/hbase-personality.sh#L562-L634 |
|
Ok, depending on @busbey's input and the level of investment necessary here it might make sense to defer that work to another jira. This PR is fixing a demonstrable bug that is cause hadoop-3.3.1-based clients to not work with the hbase client. I'd be happy to add a quick test in the appropriate place with some guidance, but I'm not sure I want to get embroiled in a larger test coverage initiative at the moment. |
|
I agree that we should not be excluding these classes. there are already checks that allow for hadoop classes to show up in just those artifacts where we allow hadoop, and AFAICT the hadoop third party stuff is properly located. the failure that HBASE-25792 was in the |
…nder hadoop 3.3.1 (#3926) Signed-off-by: Sean Busbey <[email protected]> (cherry picked from commit cd45cad)
…nder hadoop 3.3.1 (#3926) Signed-off-by: Sean Busbey <[email protected]> (cherry picked from commit cd45cad)
…nder hadoop 3.3.1 (#3926) Signed-off-by: Sean Busbey <[email protected]> (cherry picked from commit cd45cad)
…nder hadoop 3.3.1 (apache#3926) Signed-off-by: Sean Busbey <[email protected]> (cherry picked from commit cd45cad) (cherry picked from commit 87e993d) Change-Id: Ia03434a27fbc0aaa1b87397e29413bdc88c5b16d
5 participants
I expected there would be more to this, but this ended up working locally without needing to modify our invariants checks. I'm continuing to look into why, but wanted to get the full build started.