Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
Awesome. Thanks @budde @brkyvz for reviews and patch improvements.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user brkyvz commented on the issue:
https://github.com/apache/spark/pull/17467
LGTM! Merging to master/branch-2.2
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde @brkyvz - Any feed back on this one please ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
Thanks for all the review comments @budde @brkyvz . Added new review
changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user budde commented on the issue:
https://github.com/apache/spark/pull/17467
Fair enough. I took another look and I think I may have been thinking of
the way things worked in an earlier revision of this code. I think the case
class is reasonable.
---
If your project is set
Github user brkyvz commented on the issue:
https://github.com/apache/spark/pull/17467
I really prefer the case class. It's used in places where we don't pass in
`SparkContext` as well
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
Thanks for the comments @budde @brkyvz . Would be adding the changes soon.
I too liked pulling the values of spark conf directly and got it working
with the private val in
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@brkyvz - Thanks for the review comments. Updated the patch, please review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user brkyvz commented on the issue:
https://github.com/apache/spark/pull/17467
@yssharma left some comments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde would you like to share your thoughts on the new changes when you
have time ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@brkyvz - Added new changes that adds -
- A case class `KinesisReadConfigurations` that adds all the kinesis read
configs in a single place
- A test class that passes the kinesis configs in
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@felixcheung : would love to get more review comments to improve the patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde - Added changes for the minor review comments and docs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/17467
@brkyvz are you ok with this PR at a high level? If yes, I could help with
review and shepherd this
---
If your project is set up for it, you can reply to this email and have your
reply
Github user budde commented on the issue:
https://github.com/apache/spark/pull/17467
@yssharma Fair enough. I'll try to get your update reviewed later today
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde - Not sure if we can exactly test the configured timeouts. I have
debugged the flow in my testcase to verify that the custom values are passed
down to the Kinesis backed block rdd.
---
If
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde : Implemented review changes and checked scala code style checks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde @brkyvz - Implemented the review changes. Please review.
- Using SparkConf for all the user parameters
- removed kinesisWait to be val instead of var
- fixed documentation
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@budde - thanks for taking time to review it. Appreciate it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user budde commented on the issue:
https://github.com/apache/spark/pull/17467
Not a Spark committer, but I've contributed to this component in the past.
I would strongly prefer an approach that avoids adding an additional parameter
to all of the Kinesis classes if the
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
Waiting for review @brkyvz . Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
Just for info, while trying to use the `sc` in the `KinesisBackedBlockRDD `
:
`- Basic reading from Kinesis *** FAILED ***
org.apache.spark.SparkException: Task not serializable
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@brkyvz - thanks for taking time to review the patch. appreciate it.
Implemented all your suggestions. Now passing a new map for the kinesis
configs and added mechanism to use the builder for
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@srowen - Could I get some love here as well. Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17467
**[Test build #3634 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3634/testReport)**
for PR 17467 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17467
**[Test build #3634 has
started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3634/testReport)**
for PR 17467 at commit
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
Can I get some feedback here please @tdas @brkyvz Thanks :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@tdas @brkyvz
- Added new changes for removing other constants hardcoded at multiple
places.
- Squashed 3 commits into single commit
---
If your project is set up for it, you can reply
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
You're a gen @HyukjinKwon ð¯ . I will wait for Tathagata and Burak's
inputs then :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17467
I am not used to Kinesis. I usually click blame button and check both the
recent code modifier and committer, e.g.,
Github user yssharma commented on the issue:
https://github.com/apache/spark/pull/17467
@HyukjinKwon What should be the next steps for this PR. Are there any
Spark-Kinesis experts who can review the patch ?
---
If your project is set up for it, you can reply to this email and have
31 matches
Mail list logo
