Skip to content
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

Add ClouderaShimVersion to unshimmed files #5004

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

tgravescs
Copy link
Collaborator

fixes #5003

This fixes test failures with error:
: java.lang.LinkageError: loader constraint violation: when resolving method "com.nvidia.spark.rapids.shims.spark311cdh.SparkShimServiceProvider$.VERSION()Lcom/nvidia/spark/rapids/ClouderaShimVersion;" the class loader (instance of org/apache/spark/util/MutableURLClassLoader) of the current class, com/nvidia/spark/rapids/shims/spark311cdh/SparkShimServiceProvider, and the class loader (instance of org/apache/spark/util/MutableURLClassLoader) for the method's defining class, com/nvidia/spark/rapids/shims/spark311cdh/SparkShimServiceProvider$, have different Class objects for the type com/nvidia/spark/rapids/ClouderaShimVersion used in the signature�[0m

We did the same thing with the Databricks shim version in earlier PR.

Signed-off-by: Thomas Graves tgraves@nvidia.com

Signed-off-by: Thomas Graves <tgraves@nvidia.com>
@tgravescs tgravescs added the bug Something isn't working label Mar 22, 2022
@tgravescs tgravescs added this to the Mar 21 - Apr 1 milestone Mar 22, 2022
@tgravescs tgravescs self-assigned this Mar 22, 2022
@tgravescs
Copy link
Collaborator Author

build

@gerashegalov
Copy link
Collaborator

gerashegalov commented Mar 22, 2022

I would like to emphasize the need for consistency because it avoids dealing with such problems one by one. I think ShimVersion and all its descendant classes should be unshimmed together. It should be no issue to unshim them because they are unrelated to the Spark codebase.

The version probing logic probably has a subtle bug that likely will become obsolete once we unshim.

@tgravescs
Copy link
Collaborator Author

I understand what you are saying but I'm not sure if you are proposing changing it here or really what exactly you are proposing to change? At this point all the ShimVersions are included here, perhaps you are saying if we can include something like com/nvidia/spark/rapids/ShimVersion and hope that the name doesn't conflict with any other classes?

Either way since this should fix issue and we are in burn down lets decide and file an issue to change in 22.06 if we want.

@gerashegalov
Copy link
Collaborator

I understand what you are saying but I'm not sure if you are proposing changing it here or really what exactly you are proposing to change? At this point all the ShimVersions are included here, perhaps you are saying if we can include something like com/nvidia/spark/rapids/ShimVersion and hope that the name doesn't conflict with any other classes?

Yes, my suggestion boils down to having
SparkShimVersion, ClouderaShimVersion, DatabricksShimVersion, EMRShimVersion, ShimVersion in the unshimmed.txt.

So IIUC we still have EMRShimVersion shimmed even after this PR.

Either way since this should fix issue and we are in burn down lets decide and file an issue to change in 22.06 if we want.

We can handle this inconsistency in a follow-up issue.

Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please file an issue for EMRShimVersion if you merge as is

@tgravescs
Copy link
Collaborator Author

we don't have a EMR shim in the code anymore, so really could just remove that, I can file followup

@tgravescs tgravescs merged commit be1873a into NVIDIA:branch-22.04 Mar 22, 2022
@tgravescs tgravescs deleted the cdhdist branch March 22, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Cloudera 3.1.1 tests fail due to ClouderaShimVersion
3 participants