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-29675][SQL] Add exception when isolationLevel is Illegal #26334
Conversation
ok to test |
@@ -177,6 +177,10 @@ class JDBCOptions( | |||
case "READ_COMMITTED" => Connection.TRANSACTION_READ_COMMITTED | |||
case "REPEATABLE_READ" => Connection.TRANSACTION_REPEATABLE_READ | |||
case "SERIALIZABLE" => Connection.TRANSACTION_SERIALIZABLE | |||
case other => throw new IllegalArgumentException( | |||
s"Invalid value `$other` for parameter `$JDBC_TXN_ISOLATION_LEVEL`. This can be " + | |||
s"`NONE`, `READ_UNCOMMITTED`, `READ_COMMITTED`, `REPEATABLE_READ` or `SERIALIZABLE`." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: plz drop s
in the head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @HyukjinKwon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cc'ing me @maropu. LGTM too
Thanks, @HyukjinKwon ! Pending Jenkins. |
@@ -1649,4 +1649,17 @@ class JDBCSuite extends QueryTest | |||
} | |||
} | |||
} | |||
|
|||
test("SPARK-29675 Add exception when isolationLevel is Illegal") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit. @ulysses-you . We don't use JIRA ID for the improvement test cases. It's used for bug fixes only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed with @dongjoon-hyun offline. I will send an email to dev mailing list to discuss and officially document this test prefix for JIRA ID, after having some more offline discussions with some other PMCs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice, thx, PMC guys!
Test build #112978 has finished for PR 26334 at commit
|
@ulysses-you Can you remove the prefix before merging? #26334 (comment) |
Thanks for your review @maropu @HyukjinKwon @dongjoon-hyun . I have Fixed it. |
Thanks, @ulysses-you ! Pending, Jenkins. |
Test build #112995 has finished for PR 26334 at commit
|
retest this please |
Jenkins, retest this please |
Test build #113020 has finished for PR 26334 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This passed all tests including SparkR.
The only failure is CRAN incoming feasibility
check which is not our issue.
* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) :
dims [product 24] do not match the length of object [0]
I'll merge this. Thank you, @ulysses-you , @maropu , @HyukjinKwon !
@@ -177,6 +177,10 @@ class JDBCOptions( | |||
case "READ_COMMITTED" => Connection.TRANSACTION_READ_COMMITTED | |||
case "REPEATABLE_READ" => Connection.TRANSACTION_REPEATABLE_READ | |||
case "SERIALIZABLE" => Connection.TRANSACTION_SERIALIZABLE | |||
case other => throw new IllegalArgumentException( | |||
s"Invalid value `$other` for parameter `$JDBC_TXN_ISOLATION_LEVEL`. This can be " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: back-ticks aren't meaningful here right?
.load() | ||
}.getMessage | ||
assert(e.contains( | ||
"Invalid value `test` for parameter `isolationLevel`. This can be " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably just check for the first sentence rather than depend on this exact entire string. But it doesn't matter much.
Oh ha sorry you merged it. The back ticks are superfluous but it doesn't matter, just leave it. |
Oops. Sorry, @srowen . :) |
Thanks for merging, @dongjoon-hyun ! |
Thanks all ! |
What changes were proposed in this pull request?
Now we use JDBC api and set an Illegal isolationLevel option, spark will throw a
scala.MatchError
, it's not friendly to user. So we should add an IllegalArgumentException.Why are the changes needed?
Make exception friendly to user.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add UT.