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-34766][SQL] Do not capture maven config for views #31856

Closed
wants to merge 2 commits into from

Conversation

ulysses-you
Copy link
Contributor

What changes were proposed in this pull request?

Skip capture maven repo config for views.

Why are the changes needed?

Due to the bad network, we always use the thirdparty maven repo to run test. e.g.,

build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxxxx

It's failed with such error msg

[info] - show-tblproperties.sql *** FAILED *** (128 milliseconds)
[info] show-tblproperties.sql
[info] Expected "...rredTempViewNames [][]", but got "...rredTempViewNames [][
[info] view.sqlConfig.spark.sql.maven.additionalRemoteRepositories xxxxx]" Result did not match for query #6
[info] SHOW TBLPROPERTIES view (SQLQueryTestSuite.scala:464)

It's not necessary to capture the maven config to view since it's a session level config.
 

Does this PR introduce any user-facing change?

No.

How was this patch tested?

manual test pass

build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxx

@github-actions github-actions bot added the SQL label Mar 17, 2021
@SparkQA
Copy link

SparkQA commented Mar 17, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40716/

@SparkQA
Copy link

SparkQA commented Mar 17, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40716/

@ulysses-you
Copy link
Contributor Author

cc @yaooqinn @maropu @cloud-fan

@yaooqinn
Copy link
Member

LGTM

@maropu
Copy link
Member

maropu commented Mar 17, 2021

@yaooqinn Have you already tried to merge a commit? If not yet, please do it for your practice.

@yaooqinn
Copy link
Member

Yes, I have taken my first shot with my own PR(which is a follow-up, and no need for the JIRA side). Let me merge this to test the full workflow.

@yaooqinn yaooqinn closed this in 48637a9 Mar 17, 2021
@yaooqinn
Copy link
Member

yaooqinn commented Mar 17, 2021

Thanks, Merged to master~

BTW, is the second run of the pr merge tool not able to change target branch

@maropu
Copy link
Member

maropu commented Mar 17, 2021

BTW, is the second run of the pr merge tool not able to change target branch

Why can't you select a target branch? It raises an error?

@yaooqinn
Copy link
Member

yaooqinn commented Mar 17, 2021

A brief history w/ running ./dev/merge_spark_pr.py for the second round after this PR is in master

Which pull request would you like to merge? (e.g. 34): 31856

In this part, I was expecting it would ask me to enter the targeting branch, but it went to master directly

Pull request 31856 is not mergeable in its current form.
Continue? (experts only!) (y/n): y

=== Pull Request #31856 ===
title	[SPARK-34766][SQL] Do not capture maven config for views
source	ulysses-you/skip-maven-config
target	master
url	https://api.github.com/repos/apache/spark/pulls/31856

Proceed with merging pull request #31856? (y/n):
``

@cloud-fan
Copy link
Contributor

after merging to master, the script will ask you if you want to merge it to other branches.

@yaooqinn
Copy link
Member

after merging to master, the script will ask you if you want to merge it to other branches.

Yeah, I have seen this.

After if I reply n to it and I regret it, is there any chance for me to go back?

@cloud-fan
Copy link
Contributor

No, now we need to create a new PR for 3.1...

@yaooqinn
Copy link
Member

Er.. I was expecting something like
image

Hi @ulysses-you, sorry for my mistake, can we make a backport to 3.1

ulysses-you added a commit to ulysses-you/spark that referenced this pull request Mar 18, 2021
### What changes were proposed in this pull request?

Skip capture maven repo config for views.

### Why are the changes needed?

Due to the bad network, we always use the thirdparty maven repo to run test. e.g.,
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxxxx
```

It's failed with such error msg
```
[info] - show-tblproperties.sql *** FAILED *** (128 milliseconds)
[info] show-tblproperties.sql
[info] Expected "...rredTempViewNames [][]", but got "...rredTempViewNames [][
[info] view.sqlConfig.spark.sql.maven.additionalRemoteRepositories xxxxx]" Result did not match for query #6
[info] SHOW TBLPROPERTIES view (SQLQueryTestSuite.scala:464)
```

It's not necessary to capture the maven config to view since it's a session level config.
 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

manual test pass
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxx
```

Closes apache#31856 from ulysses-you/skip-maven-config.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Kent Yao <[email protected]>
@ulysses-you
Copy link
Contributor Author

created #31879

cloud-fan pushed a commit that referenced this pull request Mar 18, 2021
backport [#31856](#31856) for branch-3.1

### What changes were proposed in this pull request?

Skip capture maven repo config for views.

### Why are the changes needed?

Due to the bad network, we always use the thirdparty maven repo to run test. e.g.,
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxxxx
```

It's failed with such error msg
```
[info] - show-tblproperties.sql *** FAILED *** (128 milliseconds)
[info] show-tblproperties.sql
[info] Expected "...rredTempViewNames [][]", but got "...rredTempViewNames [][
[info] view.sqlConfig.spark.sql.maven.additionalRemoteRepositories xxxxx]" Result did not match for query #6
[info] SHOW TBLPROPERTIES view (SQLQueryTestSuite.scala:464)
```

It's not necessary to capture the maven config to view since it's a session level config.
 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

manual test pass
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxx
```

Closes #31856 from ulysses-you/skip-maven-config.

Authored-by: ulysses-you <ulyssesyou18gmail.com>
Signed-off-by: Kent Yao <yaoapache.org>

Closes #31879 from ulysses-you/SPARK-34766-3-1.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@ulysses-you ulysses-you deleted the skip-maven-config branch April 14, 2021 05:46
flyrain pushed a commit to flyrain/spark that referenced this pull request Sep 21, 2021
backport [apache#31856](apache#31856) for branch-3.1

### What changes were proposed in this pull request?

Skip capture maven repo config for views.

### Why are the changes needed?

Due to the bad network, we always use the thirdparty maven repo to run test. e.g.,
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxxxx
```

It's failed with such error msg
```
[info] - show-tblproperties.sql *** FAILED *** (128 milliseconds)
[info] show-tblproperties.sql
[info] Expected "...rredTempViewNames [][]", but got "...rredTempViewNames [][
[info] view.sqlConfig.spark.sql.maven.additionalRemoteRepositories xxxxx]" Result did not match for query apache#6
[info] SHOW TBLPROPERTIES view (SQLQueryTestSuite.scala:464)
```

It's not necessary to capture the maven config to view since it's a session level config.
 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

manual test pass
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxx
```

Closes apache#31856 from ulysses-you/skip-maven-config.

Authored-by: ulysses-you <ulyssesyou18gmail.com>
Signed-off-by: Kent Yao <yaoapache.org>

Closes apache#31879 from ulysses-you/SPARK-34766-3-1.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
fishcus pushed a commit to fishcus/spark that referenced this pull request Jan 12, 2022
backport [apache#31856](apache#31856) for branch-3.1

### What changes were proposed in this pull request?

Skip capture maven repo config for views.

### Why are the changes needed?

Due to the bad network, we always use the thirdparty maven repo to run test. e.g.,
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxxxx
```

It's failed with such error msg
```
[info] - show-tblproperties.sql *** FAILED *** (128 milliseconds)
[info] show-tblproperties.sql
[info] Expected "...rredTempViewNames [][]", but got "...rredTempViewNames [][
[info] view.sqlConfig.spark.sql.maven.additionalRemoteRepositories xxxxx]" Result did not match for query apache#6
[info] SHOW TBLPROPERTIES view (SQLQueryTestSuite.scala:464)
```

It's not necessary to capture the maven config to view since it's a session level config.
 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

manual test pass
```
build/sbt "test:testOnly *SQLQueryTestSuite" -Dspark.sql.maven.additionalRemoteRepositories=xxx
```

Closes apache#31856 from ulysses-you/skip-maven-config.

Authored-by: ulysses-you <ulyssesyou18gmail.com>
Signed-off-by: Kent Yao <yaoapache.org>

Closes apache#31879 from ulysses-you/SPARK-34766-3-1.

Authored-by: ulysses-you <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants