[SPARK-35226][SQL][FOLLOWUP] Fix test added in SPARK-35226 for DB2KrbIntegrationSuite #32632

sarutak · 2021-05-22T07:53:43Z

What changes were proposed in this pull request?

This PR fixes an test added in SPARK-35226 (#32344).

Why are the changes needed?

SELECT 1 seems non-valid query for DB2.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

DB2KrbIntegrationSuite passes on my laptop.

I also confirmed all the KrbIntegrationSuites pass with the following command.

build/sbt -Phive -Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite"

sarutak · 2021-05-22T07:54:15Z

...tegration-tests/src/test/scala/org/apache/spark/sql/jdbc/DockerKrbJDBCIntegrationSuite.scala

-
-        // Set the authentic krb5.conf but doesn't refresh config
-        // so this assertion is expected to fail.
-        intercept[Exception] {


DB2IntegrationSuite doesn't throw exception here so I'll remove this assertion.

Why does db2 throw no exception in this test? cc: @gaborgsomogyi

Anyway, since the other krb tests (e.g., PostgresKrbIntegrationSuite) refer this test, I think we cannot remove it simply.

If refreshKrb5Config is not set to true, whether krb5.conf is always read or not when a new connection is established can depend on the implementation for each JDBC driver.
We can remove the assertion because the remaining two assertions are important.
The second assertion ensures that refreshKrb5Config enforces to refresh.

You mean DB2KrbIntegrationSuite instead of DB2IntegrationSuite, right?

DB2IntegrationSuite doesn't throw exception here so I'll remove this assertion.

I also confirmed that DB2KrbIntegrationSuite fails

[info] - SPARK-35226: JDBCOption should accept refreshKrb5Config parameter *** FAILED *** (1 second, 147 milliseconds) [info] Expected exception java.lang.Exception to be thrown, but no exception was thrown (DockerKrbJDBCIntegrationSuite.scala:187) [info] org.scalatest.exceptions.TestFailedException:

Ah, Yes. I mean DB2KrbIntegrationSuite.

sarutak · 2021-05-22T07:54:32Z

cc: @dongjoon-hyun

maropu · 2021-05-22T08:20:30Z

...tegration-tests/src/test/scala/org/apache/spark/sql/jdbc/DockerKrbJDBCIntegrationSuite.scala

@@ -200,11 +188,11 @@ abstract class DockerKrbJDBCIntegrationSuite extends DockerJDBCIntegrationSuite
          .option("keytab", keytabFullPath)
          .option("principal", principal)
          .option("refreshKrb5Config", "true")
-          .option("query", "SELECT 1")
+          .option("query", "SELECT c0 FROM bar")


could you use dbtable instead?

SparkQA · 2021-05-22T09:00:20Z

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

SparkQA · 2021-05-22T09:32:17Z

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

SparkQA · 2021-05-22T09:58:35Z

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

SparkQA · 2021-05-22T10:29:37Z

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

SparkQA · 2021-05-22T10:43:38Z

Test build #138825 has finished for PR 32632 at commit 29031c2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-05-22T11:52:24Z

Test build #138827 has finished for PR 32632 at commit 3f9b37f.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2021-05-22T12:41:46Z

retest this please.

SparkQA · 2021-05-22T14:00:41Z

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

SparkQA · 2021-05-22T14:34:31Z

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

SparkQA · 2021-05-22T15:35:58Z

Test build #138830 has finished for PR 32632 at commit 3f9b37f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2021-05-23T02:54:30Z

Although this is a newly added test case issue, do you think this is a release blocker, @sarutak ?

sarutak · 2021-05-23T05:25:30Z

@dongjoon-hyun
DB2KrbIntegrationSuite always fails so It's better to fix this issue but I don't think it's a blocker because this issue doesn't break any feature.

HyukjinKwon · 2021-05-23T05:25:54Z

Yeah, I think we don't have to block the release.

dongjoon-hyun

+1, LGTM. Thank you, @sarutak and @maropu .

For the removed test coverage, I filed SPARK-35491.

Since this PR is a recovery of failed test case, I'll merge this first.

…IntegrationSuite ### What changes were proposed in this pull request? This PR fixes an test added in SPARK-35226 (#32344). ### Why are the changes needed? `SELECT 1` seems non-valid query for DB2. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? DB2KrbIntegrationSuite passes on my laptop. I also confirmed all the KrbIntegrationSuites pass with the following command. ``` build/sbt -Phive -Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` Closes #32632 from sarutak/followup-SPARK-35226. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 1a43415) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2021-05-23T05:33:07Z

Oh, thank you, @HyukjinKwon ! I missed your comments while writing a review comments.

HyukjinKwon · 2021-05-23T05:34:35Z

Nono nothing to do with my comment. No worries 👍

…IntegrationSuite ### What changes were proposed in this pull request? This PR fixes an test added in SPARK-35226 (apache#32344). ### Why are the changes needed? `SELECT 1` seems non-valid query for DB2. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? DB2KrbIntegrationSuite passes on my laptop. I also confirmed all the KrbIntegrationSuites pass with the following command. ``` build/sbt -Phive -Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` Closes apache#32632 from sarutak/followup-SPARK-35226. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 1a43415) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

Fix for DB2KrbIntegrationSuite.

29031c2

github-actions bot added the SQL label May 22, 2021

sarutak commented May 22, 2021

View reviewed changes

maropu reviewed May 22, 2021

View reviewed changes

Use dbtable.

3f9b37f

dongjoon-hyun approved these changes May 23, 2021

View reviewed changes

dongjoon-hyun closed this in 1a43415 May 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-35226][SQL][FOLLOWUP] Fix test added in SPARK-35226 for DB2KrbIntegrationSuite #32632

[SPARK-35226][SQL][FOLLOWUP] Fix test added in SPARK-35226 for DB2KrbIntegrationSuite #32632

sarutak commented May 22, 2021 •

edited

sarutak May 22, 2021

maropu May 22, 2021

maropu May 22, 2021

sarutak May 22, 2021 •

edited

dongjoon-hyun May 23, 2021

sarutak May 23, 2021

sarutak commented May 22, 2021 •

edited

maropu May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

sarutak commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

dongjoon-hyun commented May 23, 2021

sarutak commented May 23, 2021 •

edited

HyukjinKwon commented May 23, 2021

dongjoon-hyun left a comment

dongjoon-hyun commented May 23, 2021

HyukjinKwon commented May 23, 2021

[SPARK-35226][SQL][FOLLOWUP] Fix test added in SPARK-35226 for DB2KrbIntegrationSuite #32632

[SPARK-35226][SQL][FOLLOWUP] Fix test added in SPARK-35226 for DB2KrbIntegrationSuite #32632

Conversation

sarutak commented May 22, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

sarutak May 22, 2021

Choose a reason for hiding this comment

maropu May 22, 2021

Choose a reason for hiding this comment

maropu May 22, 2021

Choose a reason for hiding this comment

sarutak May 22, 2021 • edited

Choose a reason for hiding this comment

dongjoon-hyun May 23, 2021

Choose a reason for hiding this comment

sarutak May 23, 2021

Choose a reason for hiding this comment

sarutak commented May 22, 2021 • edited

maropu May 22, 2021

Choose a reason for hiding this comment

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

sarutak commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

SparkQA commented May 22, 2021

dongjoon-hyun commented May 23, 2021

sarutak commented May 23, 2021 • edited

HyukjinKwon commented May 23, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented May 23, 2021

HyukjinKwon commented May 23, 2021

sarutak commented May 22, 2021 •

edited

sarutak May 22, 2021 •

edited

sarutak commented May 22, 2021 •

edited

sarutak commented May 23, 2021 •

edited