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

[SPARK-44594][SS] Remove redundant method parameter in kafka connector #42198

Closed
wants to merge 1 commit into from

Conversation

zhaomin1423
Copy link
Member

What changes were proposed in this pull request?

There are have redundant parameters in org.apache.spark.sql.kafka010.KafkaWriter#validateQuery and org.apache.spark.sql.kafka010.KafkaWriter#write, can remove them.

Why are the changes needed?

They are not used, remove them to make the code more concise.

Does this PR introduce any user-facing change?

no

How was this patch tested?

Existing can test it.

@zhaomin1423
Copy link
Member Author

After run ./dev/scalafmt, some unmodified code is formatted.

@HyukjinKwon
Copy link
Member

cc @HeartSaVioR FYI

@LuciferYang
Copy link
Contributor

After run ./dev/scalafmt, some unmodified code is formatted.

Please don't do this, scalafmt is only used to format the connect module

@zhaomin1423
Copy link
Member Author

After run ./dev/scalafmt, some unmodified code is formatted.

Please don't do this, scalafmt is only used to format the connect module

Thanks, I push again. But the docs may be confusing here.

https://github.com/apache/spark-website/blob/d865ca2798b67209dbd78326af9085355dc74b0f/contributing.md?plain=1#L457

@LuciferYang
Copy link
Contributor

@zhaomin1423 Sorry for the confusion, to be precise, only the connect modules needs to be forced to execute scalafmt, for other modules, we should avoid making unrelated modifications to keep the changes as minimal as possible.

@LuciferYang
Copy link
Contributor

On the other hand, could you file a new JIRA for this pr?

@zhaomin1423
Copy link
Member Author

@zhaomin1423 Sorry for the confusion, to be precise, only the connect modules needs to be forced to execute scalafmt, for other modules, we should avoid making unrelated modifications to keep the changes as minimal as possible.

Thanks for the explanation, I'm a newcomer. Do we need to update the description of the documentation to avoid misunderstanding for newcomers.

@zhaomin1423
Copy link
Member Author

On the other hand, could you file a new JIRA for this pr?

Yes, I don't have a permission, have already applied, waiting for approval.

@zhaomin1423 zhaomin1423 changed the title Remove redundant method parameter in kafka sink [SPARK-44594][KAFKA] Remove redundant method parameter in kafka connector Jul 29, 2023
@zhaomin1423
Copy link
Member Author

zhaomin1423 commented Jul 29, 2023

On the other hand, could you file a new JIRA for this pr?

Yes, I don't have a permission, have already applied, waiting for approval.

I file and edit the title, PTAL. https://issues.apache.org/jira/browse/SPARK-44594

@LuciferYang
Copy link
Contributor

Although these two APIs are limited to the kafka010 package, I also searched GitHub and it seems that no other project uses these two APIs. Perhaps modifying them is safe, but it is best to follow @HeartSaVioR 's decision

@PhilDakin
Copy link
Contributor

@LuciferYang what decision are you referring to here? I was unable to find anything in the mail list.

Should this PR + corresponding Jira be closed?

@HyukjinKwon HyukjinKwon changed the title [SPARK-44594][KAFKA] Remove redundant method parameter in kafka connector [SPARK-44594][SS] Remove redundant method parameter in kafka connector Oct 12, 2023
Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

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

+1, LGTM

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1
This is not a user facing change so doesn't need to be future-proofing. Whenever we want to verify the query with params we can pass the param again.

@LuciferYang
Copy link
Contributor

Merged into master for Spark 4.0. Thanks @zhaomin1423 @HeartSaVioR and @HyukjinKwon

@zhaomin1423
Copy link
Member Author

Merged into master for Spark 4.0. Thanks @zhaomin1423 @HeartSaVioR and @HyukjinKwon

Thanks for your help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants