Skip to content

Conversation

@alkis
Copy link
Contributor

@alkis alkis commented Mar 16, 2022

What changes were proposed in this pull request?

Rename the external top level directory to connector.

Why are the changes needed?

external is a hardwired special name for bazel and this causes all sorts of issues with both native bazel or extensions like bazel-compile-commands-extractor. Spark forks using bazel to build Spark have to go through hoops to make things work if at all.

Amongst other things, making the source code browsable through top level external symlink is impossible with a top level external directory in the repository.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Ran ./build/mvn -DskipTests clean package and ./dev/run-tests.

@alkis alkis changed the title Move external to vendor Rename external to vendor top level dir Mar 16, 2022
@alkis alkis marked this pull request as ready for review March 16, 2022 10:28
@cloud-fan
Copy link
Contributor

It makes sense to avoid using reserved name for some build systems. Can we create a JIRA ticket for it?

@alkis alkis changed the title Rename external to vendor top level dir [SPARK-38569][BUILD] Rename external to vendor top level dir Mar 16, 2022
@alkis
Copy link
Contributor Author

alkis commented Mar 16, 2022

It makes sense to avoid using reserved name for some build systems. Can we create a JIRA ticket for it?

Sure thing. Done.

@alkis
Copy link
Contributor Author

alkis commented Mar 16, 2022

FYI for more details on how this causes major issues in practice hedronvision/bazel-compile-commands-extractor#30

Copy link
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

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

LGTM, can you fix the merge conflcts?

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Since this is big, shall we discuss a little more in the dev mailing list before merging, please?

@dongjoon-hyun
Copy link
Member

Although Apache Spark is irrelevant to bazel, I fully understand the requirement from downstream projects here.
What we need is simply an officially discussion with a broader community audience.

@alkis
Copy link
Contributor Author

alkis commented Mar 17, 2022

Although Apache Spark is irrelevant to bazel, I fully understand the requirement from downstream projects here. What we need is simply an officially discussion with a broader community audience.

Thank you Dongjoon. This is my first contribution to Apache Spark so bear with my perhaps silly questions. Could you advice what topics we want to discuss - the move itself, the final directory name or something else? While the PR looks big, at its core this is a single directory rename (maybe there are risks I am unaware of?).

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@dongjoon-hyun
Copy link
Member

@alkis .

You are asking a broader upstream breaking restructuring for your specific build system's limitation. Your suggestion might cause another conflicts and headache on another downstream forks.

So, please send an email about your intention and share this PR link to dev@spark mailing list first and build a consensus among the other community members. If there is any objections, we need a discussion, don't we?

If we agree to rename, the next step is the consensus on new name. From my side, I don't think vendor is a correct usage here. Why Apache Spark's Avro and Kafka module should be under vendor? Does it make sense?

BTW, although bezel is not supported by Apache Spark and the community recommend to use Maven and SBT so far, I'm not against your use case of bazel.

@alkis
Copy link
Contributor Author

alkis commented Mar 17, 2022

Thank you Dongjoon. Makes sense. I started a conversation at dev@spark mailing list.

@dongjoon-hyun
Copy link
Member

Could you revise the PR title and description according to the PR content?

@alkis alkis changed the title [SPARK-38569][BUILD] Rename external to vendor top level dir [SPARK-38569][BUILD] Rename external to connector top level dir Mar 24, 2022
@alkis alkis changed the title [SPARK-38569][BUILD] Rename external to connector top level dir [SPARK-38569][BUILD] Rename external top level dir to connector Mar 24, 2022
@alkis
Copy link
Contributor Author

alkis commented Mar 24, 2022

Could you revise the PR title and description according to the PR content?

Done.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in b112528 Mar 25, 2022
@alkis
Copy link
Contributor Author

alkis commented Mar 25, 2022

thanks, merging to master!

Woohoo, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants