-
Notifications
You must be signed in to change notification settings - Fork 58
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
PHOENIX-5197: Use extraOptions to set the configuration for Spark Workers #2
Conversation
d39b930
to
dff2c4c
Compare
Is there a way to add a test to verify that if you pass in a property in via the options in the datasource it will be used? |
A couple of problems that we will run into:
|
@ChinmaySKulkarni After talking offline, there isn't a clean way to test this, so its fine to not include a test. Regarding DataSourceOptions converting all the keys to lower case, we should come up with a serialization format to store the key value pairs as a value so that we can preserve the case. |
@@ -51,7 +53,7 @@ public PhoenixDataWriter(PhoenixDataSourceWriteOptions options) { | |||
String scn = options.getScn(); | |||
String tenantId = options.getTenantId(); | |||
String zkUrl = options.getZkUrl(); | |||
Properties overridingProps = new Properties(); | |||
Properties overridingProps = options.getOverriddenProps(); |
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.
Can options.getOverriddenProps return null?
@@ -77,14 +80,15 @@ private QueryPlan getQueryPlan() throws SQLException { | |||
String scn = options.getScn(); | |||
String tenantId = options.getTenantId(); | |||
String zkUrl = options.getZkUrl(); | |||
Properties overridingProps = new Properties(); | |||
Properties overridingProps = options.getOverriddenProps(); |
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 think options.getOverriddenProps can never be null here, right?
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.
Ideally they can never be null, but added a null-check to avoid any problems in the future. Same for the write path.
dff2c4c
to
f193ada
Compare
@twdsilva Please review now. I have added a unit test and separate logging properties for IT, src and test directories as well. |
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.
+1 LGTM, I just had one minor comment.
...c/main/java/org/apache/phoenix/spark/datasource/v2/writer/PhoenixDataSourceWriteOptions.java
Outdated
Show resolved
Hide resolved
f193ada
to
7fe63c5
Compare
Thanks for the review @twdsilva. I will commit this today. |
No description provided.