-
Notifications
You must be signed in to change notification settings - Fork 29k
SPARK-5960 allow aws credentials to be passed to KinesisUtils.createStream #5439
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
Conversation
extras/kinesis-asl/pom.xml
Outdated
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.
(Why is this needed?)
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.
tests are failing on kinesis-acl, at least on my local. Error message is saying a method is missing from a class in jackson library. I don't know the exact reason but adding this dependency fixes the error
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.
Hm, I doubt this is the right way to fix it, since you are declaring a compile-time dependency on something that you don't use. Surely it's because there is a missing dep on some Kinesis component at runtime scope? you also shouldn't manage versions here.
|
Can one of the admins verify this patch? |
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: space after import.
I know we have all lived to regret including third-party classes as part of the API. Although it seems maybe essential here, I wonder if there is any reasonable way to not accept this as AWSCredentials but as access keys or something. I suppose that this could work but would assume a certain type of credentials are being used.
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.
Updated code a bit after your comments.
Created Credentials class to avoid third-party import in api.
Moved credentials parameter to existing constructor as an optional parameter.
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.
my initial attempt at adding this support introduced a BasicAWSCredentialsProvider as follows:
highlights:
- takes AccessKey and SecretKey as Strings
- follows the same hierarchy as the rest of the AWSCredentialsProviders (including not being Serializable)
|
ok to test |
|
Test build #30028 has started for PR 5439 at commit |
|
Test build #30028 has finished for PR 5439 at commit
|
|
Test FAILed. |
|
Test build #30115 has started for PR 5439 at commit |
|
Test build #30115 has finished for PR 5439 at commit
|
|
Test PASSed. |
|
@srowen tests passed, fyi |
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.
collapse the IRecordProcessor and IRecordProcessorFactory into a single import, 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.
done
|
Test build #30334 has started for PR 5439 at commit |
|
Test build #30334 has finished for PR 5439 at commit
|
|
Test FAILed. |
|
Test build #30335 has started for PR 5439 at commit |
|
Test build #30335 has finished for PR 5439 at commit
|
|
Test PASSed. |
|
Test build #30406 has started for PR 5439 at commit |
|
Test build #30406 has finished for PR 5439 at commit
|
|
Test PASSed. |
|
@cfregly Moved initialization code into |
|
@cfregly any other comments? |
|
Can one of the admins verify this patch? |
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 am really not sure how this will work. AWSCredentials is not serializable, while the Receiver has to be serializable (as the receiver object is sent to the executors from the driver). Did you test whether this works?
|
I am taking a look at this PR a little late. But I am not sure how this work at all. AWSCredentials is not serializable. |
|
@mce Are you able to update this? If you are not able to work on this, mind closing this PR? |
|
@tdas sorry for the late response, i'm closing the pr now as i don't have any time to work on this currently. |
No description provided.