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

[BUG] Documented deployment of spark-avro is not tested #5657

Closed
Tracked by #5757
gerashegalov opened this issue May 26, 2022 · 8 comments · Fixed by #6505
Closed
Tracked by #5757

[BUG] Documented deployment of spark-avro is not tested #5657

gerashegalov opened this issue May 26, 2022 · 8 comments · Fixed by #6505
Assignees
Labels
bug Something isn't working P0 Must have for release reliability Features to improve reliability or bugs that severly impact the reliability of the plugin

Comments

@gerashegalov
Copy link
Collaborator

Describe the bug
The documented way of deploying spark-avro which we are likely to encounter in the field is by having Spark load it at run time via --packages option from Maven.

However CI only tests with static classpath using spark.*.extraClassPath option.

Steps/Code to reproduce bug
run intergration tests with -k avro with TEST_PARALLEL > 2

Expected behavior
Prioritize testing the advertised way of deploying --packages spark-avro if we need to pick one.

Environment details (please complete the following information)
CI

Additional context
#5648

@gerashegalov gerashegalov added bug Something isn't working ? - Needs Triage Need team to review and classify P0 Must have for release reliability Features to improve reliability or bugs that severly impact the reliability of the plugin labels May 26, 2022
@sameerz sameerz removed the ? - Needs Triage Need team to review and classify label Jun 7, 2022
@pxLi
Copy link
Collaborator

pxLi commented Jul 13, 2022

This sounds more like spark thing to me instead of our plugin one?

--packages                  Comma-separated list of maven coordinates of jars to include
                              on the driver and executor classpaths. Will search the local
                              maven repo, then maven central and any additional remote
                              repositories given by --repositories. The format for the
                              coordinates should be groupId:artifactId:version.

Even it failed, we should file ticket to spark :)

Also can you provide the detailed --packages=xxxx, like which avro pkg are we expecting here.
And our https://github.com/NVIDIA/spark-rapids/blob/branch-22.08/integration_tests/run_pyspark_from_build.sh does not support packages deployment yet, we may need to do some refactor. Do we really need to cover this one?

Thanks!

@GaryShen2008
Copy link
Collaborator

Hi @gerashegalov,
Do we need to run all the avro test cases in integration test to check the --packages deploy way for Avro reading?
If not, I wonder if we just set up a CI job to run a spark job with Avro reading.
What's the point of using TEST_PARALLEL > 2?

@gerashegalov
Copy link
Collaborator Author

This sounds more like spark thing to me instead of our plugin one?

Even it failed, we should file ticket to spark :)
Not really.

This would be the case only for bugs that reproduce even without our Plugin activated. However, our plugin has code affected by this feature and it has exhibited bugs unique to our plugin.

We obviously file and fix upstream issues when appropriate.

The goal of this issue to proactively discover the issues our users are likely to hit before they do and come up with workarounds, fixes for the Plugin and upstream Spark when necessary.

Also can you provide the detailed --packages=xxxx, like which avro pkg are we expecting here. And our https://github.com/NVIDIA/spark-rapids/blob/branch-22.08/integration_tests/run_pyspark_from_build.sh does not support packages deployment yet, we may need to do some refactor. Do we really need to cover this one?

It's documented in the upstream spark-avro doc link provided in the issue description

org.apache.spark:spark-avro_2.12:sparkVersion

@gerashegalov
Copy link
Collaborator Author

Hi @gerashegalov, Do we need to run all the avro test cases in integration test to check the --packages deploy way for Avro reading?

it's always a matter how hard you want to think if you missed anything. So the usual trade-off of test coverage vs resource spend on testing.

If not, I wonder if we just set up a CI job to run a spark job with Avro reading.

yes, this is a minimum viable solution, again a matter of coverage whether it will have all the elements we cover in integration tests. IMO factoring out the Avro integration tests into a dedicated sub-pipeline paramerized on deployment options would be ideal. An MVP solution is a good start though.

What's the point of using TEST_PARALLEL > 2?

This is an interesting artifact tracked in #5714 . We deploy jars differently based on TEST_PARALLEL

@pxLi
Copy link
Collaborator

pxLi commented Aug 1, 2022

@gerashegalov @sameerz as iceberg test include --packages method https://github.com/NVIDIA/spark-rapids/blob/branch-22.08/jenkins/spark-tests.sh#L187
which covers some sanity check for this issue, do you guys mind we move the ticket to 22.10 for now?

@sameerz
Copy link
Collaborator

sameerz commented Aug 1, 2022

I am ok to move this to 22.10.

@gerashegalov
Copy link
Collaborator Author

I am fine moving it, too, because it's not a regression.

@pxLi
Copy link
Collaborator

pxLi commented Aug 2, 2022

thanks! @zhanga5 will help look into this later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P0 Must have for release reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants