Skip to content

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

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.

Sign up for GitHub

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

Closed
sarutak wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29031c2
Fix for DB2KrbIntegrationSuite.
sarutak May 22, 2021
3f9b37f
Use dbtable.
sarutak May 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions ...ration-tests/src/test/scala/org/apache/spark/sql/jdbc/DockerKrbJDBCIntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,7 @@ abstract class DockerKrbJDBCIntegrationSuite extends DockerJDBCIntegrationSuite
.option("keytab", keytabFullPath)
.option("principal", principal)
.option("refreshKrb5Config", "true")
.option("query", "SELECT 1")
.load()
}

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

@sarutak sarutak May 22, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@maropu maropu May 22, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@maropu maropu May 22, 2021

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@sarutak sarutak May 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun May 23, 2021

Choose a reason for hiding this comment

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

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:

Copy link
Copy Markdown
Member Author

@sarutak sarutak May 23, 2021

Choose a reason for hiding this comment

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

Ah, Yes. I mean DB2KrbIntegrationSuite.

sys.props(KRB5_CONF_PROP) = origKrb5Conf
spark.read.format("jdbc")
.option("url", jdbcUrl)
.option("keytab", keytabFullPath)
.option("principal", principal)
.option("query", "SELECT 1")
.option("dbtable", "bar")
.load()
}

Expand All @@ -200,11 +188,11 @@ abstract class DockerKrbJDBCIntegrationSuite extends DockerJDBCIntegrationSuite
.option("keytab", keytabFullPath)
.option("principal", principal)
.option("refreshKrb5Config", "true")
.option("query", "SELECT 1")
.option("dbtable", "bar")
.load()
val result = df.collect().map(_.getInt(0))
val result = df.collect().map(_.getString(0))
assert(result.length === 1)
assert(result(0) === 1)
assert(result(0) === "hello")
} finally {
sys.props(KRB5_CONF_PROP) = origKrb5Conf
}
Expand Down