Skip to content

Comments

[SPARK-38430][K8S][DOCS] Add SBT commands to K8s IT README#35745

Closed
williamhyun wants to merge 2 commits intoapache:masterfrom
williamhyun:sbtdoc
Closed

[SPARK-38430][K8S][DOCS] Add SBT commands to K8s IT README#35745
williamhyun wants to merge 2 commits intoapache:masterfrom
williamhyun:sbtdoc

Conversation

@williamhyun
Copy link
Member

@williamhyun williamhyun commented Mar 7, 2022

What changes were proposed in this pull request?

This PR aims to add SBT commands to K8s IT README.

Why are the changes needed?

This will introduce new SBT commands to developers.

Does this PR introduce any user-facing change?

No, this is a dev-only change.

How was this patch tested?

Manual.

@williamhyun williamhyun changed the title [SPARK-38430][K8S][DOCS] Add SBT commands to K8s IT readme [SPARK-38430][K8S][DOCS] Add SBT commands to K8s IT README Mar 7, 2022
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-38430][K8S][DOCS] Add SBT commands to K8s IT README [SPARK-38430][K8S][DOCS] Add SBT commands to K8s IT README Mar 7, 2022
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.

+1, LGTM. Thank you, @williamhyun .
Merged to master/3.2.

dongjoon-hyun pushed a commit that referenced this pull request Mar 7, 2022
### What changes were proposed in this pull request?
This PR aims to add SBT commands to K8s IT README.

### Why are the changes needed?
This will introduce new SBT commands to developers.

### Does this PR introduce _any_ user-facing change?
No, this is a dev-only change.

### How was this patch tested?
Manual.

Closes #35745 from williamhyun/sbtdoc.

Authored-by: William Hyun <william@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 3bbc43d)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

I am a bit late with the review - the PR is merged already. I hope the comments are useful anyway.

-Dspark.kubernetes.test.imageTag=2022-03-06 \
'kubernetes-integration-tests/test'

The following is an example to rerun tests with the pre-built image.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this documentation!

It is not very clear to me what is the difference between .../test and .../runIts ?
Is it that the first one (re-)builds the image and the second would fail if there is no image ?

Copy link
Member

Choose a reason for hiding this comment

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

Your understanding is correct and consistent with what @williamhyun described.

Copy link
Member

Choose a reason for hiding this comment

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

To make it sure,

  • ../test is slower and sometimes accidentally overwrite the images.
  • ../runIts is faster and guarantees that there will be no new image.


You can use SBT in the same way to build image and run all K8s integration tests except Minikube-only ones.

build/sbt -Psparkr -Pkubernetes -Pkubernetes-integration-tests \
Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to this PR but for ./build/mvn one has to add a profile for Hadoop 2/3 (e.g. -Phadoop-3). Otherwise the build fails due to missing AWS classes.

Copy link
Member

Choose a reason for hiding this comment

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

@martin-g . Why do you add your irrelevant comments like that? Actually, it doesn't look like a good way to collaborate because this is open to all the community member and your comments are distracting the the other people from the PR content itself.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to recommend you to file a JIRA officially instead and go for it. :)

@martin-g
Copy link
Member

martin-g commented Mar 7, 2022

Sorry for commenting on closed PR but there was no time for others to review it. It was closed very soon after being opened.

I just tried to say that the new documentation was not clear enough for me (a newbie). I had to read it few times and to consult with the code to understand the difference. I was hoping that it might be improved.

I noticed that the PR is closed when I was finishing the code review - there were no radio buttons to Comment/Approve/Request changes.

I will file tickets for both issues!

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 7, 2022

It seems that you misunderstand the point. Apache Spark uses post-commit review also actively. That's totally fine and helpful.

My comment is about only your irrelevant comment, #35745 (comment) . That's no good, isn't it?

Not directly related to this PR but for ./build/mvn one has to add a profile for Hadoop 2/3 (e.g. -Phadoop-3). Otherwise the build fails due to missing AWS classes.

kazuyukitanimura pushed a commit to kazuyukitanimura/spark that referenced this pull request Aug 10, 2022
### What changes were proposed in this pull request?
This PR aims to add SBT commands to K8s IT README.

### Why are the changes needed?
This will introduce new SBT commands to developers.

### Does this PR introduce _any_ user-facing change?
No, this is a dev-only change.

### How was this patch tested?
Manual.

Closes apache#35745 from williamhyun/sbtdoc.

Authored-by: William Hyun <william@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 3bbc43d)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 7eafadb)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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.

3 participants