[SPARK-20189][DStream] Fix spark kinesis testcases to remove deprecated createStream and use Builders #17506

yssharma · 2017-04-01T23:31:46Z

What changes were proposed in this pull request?

The spark-kinesis testcases use the KinesisUtils.createStream which are deprecated now. Modify the testcases to use the recommended KinesisInputDStream.builder instead.
This change will also enable the testcases to automatically use the session tokens automatically.

How was this patch tested?

All the existing testcases work fine as expected with the changes.

https://issues.apache.org/jira/browse/SPARK-20189

srowen

My general comment is that we don't want to leave even deprecated methods untested.

srowen · 2017-04-02T09:18:55Z

...nal/kinesis-asl/src/test/java/org/apache/spark/streaming/kinesis/JavaKinesisStreamSuite.java

This is now testing a Scala API in a Java test?

I agree. I however couldn't pass the org.apache.spark.api.java.function.Function to the buildWithHandler. Tried lot of other ways but only this seemed to make the api happy.

I will add more testcases for the deprecated API function createStream

The old style testcases do not work with the aws session tokens. I could add a ENV variable based check if that seems like a better solution ?

I don't think this is a change you should make then. If you're switching to Scala in a Java test to avoid a deprecated API then the test isn't quite testing what it is meant to. It also sounds like it could potentially leave the existing deprecated method untested.

Are you saying there's not an un-deprecated way to call this in the Java API? That sounds like a closely related issue that should be addressed first if so.

yssharma · 2017-04-03T09:52:19Z

@srowen - Have made following improvements to the patch to incorporate your points:

Did not remove old testcases for keeping the checks on old api
Added new testcases that test the new kinesis api
Added an option to tag the old api testcases with flag (which can also be useful for identifying them in future easily)
Added option to set env variable to disable the old api kinesis tests - this will help devs to run the complete testsuites on aws-session-token based infrastructure without any changes

Let me know your thoughts. Thanks.

srowen · 2017-04-03T10:35:28Z

So, is the point just removing calls to a deprecated API? It doesn't seem like it needs to be this complicated.

If a test is not specifically intended to test the deprecated API, then it should just change to use the newer undeprecated API. If that doesn't really exist, don't change that instance. Later an undeprecated alternative ought to be added, really, for callers to use as well as tests.

yssharma · 2017-04-03T11:13:38Z

@srowen - Yes, 2 objectives for the patch:

add new testcases for new api (that also tests aws session tokens)
ability to disable old api test cases

For now I have added new test cases for the new api, so that we have the coverage for session tokens.
I could remove the part of disabling old test cases, but then while running the whole test suites there would be few failures for environments with session tokens and assumed roles, and devs might think why do we have broken test cases.

I could remove the flag option, and just keep the new test cases if that suits better. Thoughts ?

Thanks

srowen · 2017-04-03T11:28:59Z

Why would you disable old API tests here? the primary purpose is not calling deprecated APIs in tests where it's not needed. Best thing is to just do that. If you happen to add a new test or two along the way to shore up coverage that's good too. I wouldn't make it more complex than that here.

- add new kinesis api testcases - add flag to disable old kinesis api testcases

SparkQA · 2017-04-03T12:14:05Z

Test build #3635 has finished for PR 17506 at commit 7f86f39.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

yssharma · 2017-04-03T12:26:05Z

@srowen - Thanks for the feedback. Appreciate it. Added a minimal simplified patch which fixes the testcases that fail with the old api.

yssharma · 2017-04-04T02:18:33Z

The Scala style check fail because of the double spaced lines probably. But that's how the existing code was so thought of keeping it that way.

srowen · 2017-04-04T09:32:51Z

@yssharma you can't check in code that fails style checks. The existing code passes the checks.

yssharma · 2017-04-04T11:02:58Z

@srowen : It failed on the earlier patch with KinesisTestUtils.scala changes. This version is clean. Will wait for the next automated build :)

Build logs: https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3635/console
Commit : 7f86f39

========================================================================
Running Scala style checks
========================================================================
Scalastyle checks failed at following occurrences:
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:220: File line length exceeds 100 characters
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:242: File line length exceeds 100 characters
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:246: File line length exceeds 100 characters
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:256: File line length exceeds 100 characters
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:219:0: Use Javadoc style indentation for multiline comments
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:227:0: Use Javadoc style indentation for multiline comments
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/external/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisTestUtils.scala:234:0: Use Javadoc style indentation for multiline comments
[error] (streaming-kinesis-asl/compile:scalastyle) errors exist
[error] Total time: 20 s, completed Apr 3, 2017 5:14:05 AM

yssharma · 2017-04-06T00:55:05Z

@srowen - does the Jenkins re-test trigger automatically?
else, could I request a retest on this patch please ?

srowen · 2017-04-06T08:16:52Z

Jenkins add to whitelist

srowen · 2017-04-06T08:17:45Z

It does retrigger automatically but you need to push commits.
Hm, it may be that somehow the kinesis code isn't passing style checks and wasn't caught? because it's not generally tested unless it's changed. In any event yes definitely fix up any errors old or new.

SparkQA · 2017-04-06T08:53:27Z

Test build #75566 has finished for PR 17506 at commit b477090.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yssharma · 2017-04-06T12:45:29Z

Thanks @srowen

yssharma · 2017-04-08T12:09:26Z

Is there anything else that can be done on this patch. The patch fixes all the deprecated api testcases that try to use the aws secret/id credentials instead of the builder.

yssharma · 2017-04-12T03:33:44Z

@srowen do you feel this patch could be merged now ?
Thanks

srowen

I don't know enough to really evaluate

srowen · 2017-04-12T07:41:27Z

external/kinesis-asl/src/test/scala/org/apache/spark/streaming/kinesis/KinesisStreamSuite.scala

-        localTestUtils.endpointUrl, localTestUtils.regionName, InitialPositionInStream.LATEST,
-        Seconds(10), StorageLevel.MEMORY_ONLY,
-        awsCredentials.getAWSAccessKeyId, awsCredentials.getAWSSecretKey)
+      val stream = KinesisInputDStream.builder.streamingContext(ssc)


Just for my understanding, it's no longer necessary to pass in the credentials explicitly?

Yes thats true. The KinesisInputDStream.builder uses the Dafault credentials if no creds are passed, and the default creds work with both AWS Key and Session tokens.
So now "mvn test" works on both permanent aws keys & session based token environments.

srowen · 2017-04-12T07:41:42Z

...rnal/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisInputDStream.scala

        checkpointInterval.getOrElse(ssc.graph.batchDuration),
        storageLevel.getOrElse(DEFAULT_STORAGE_LEVEL),
-        handler,
+        ssc.sc.clean(handler),


Is this related?

Yes this is required.
KinesisUtils used to send a cleaned Handler while creating stream, so we do this in the KinesisInputDstream now.

val cleanedHandler = ssc.sc.clean(messageHandler)

srowen · 2017-04-13T07:49:41Z

Merged to master

yssharma · 2017-04-14T22:43:47Z

Thanks @srowen

…ed createStream and use Builders ## What changes were proposed in this pull request? The spark-kinesis testcases use the KinesisUtils.createStream which are deprecated now. Modify the testcases to use the recommended KinesisInputDStream.builder instead. This change will also enable the testcases to automatically use the session tokens automatically. ## How was this patch tested? All the existing testcases work fine as expected with the changes. https://issues.apache.org/jira/browse/SPARK-20189 Author: Yash Sharma <[email protected]> Closes apache#17506 from yssharma/ysharma/cleanup_kinesis_testcases.

srowen reviewed Apr 2, 2017

View reviewed changes

yssharma force-pushed the ysharma/cleanup_kinesis_testcases branch 2 times, most recently from 84d5352 to 7f86f39 Compare April 3, 2017 09:47

Kinesis Testcases

b477090

- add new kinesis api testcases - add flag to disable old kinesis api testcases

yssharma force-pushed the ysharma/cleanup_kinesis_testcases branch from 7f86f39 to b477090 Compare April 3, 2017 12:24

srowen reviewed Apr 12, 2017

View reviewed changes

asfgit closed this in ec68d8f Apr 13, 2017

[SPARK-20189][DStream] Fix spark kinesis testcases to remove deprecated createStream and use Builders #17506

[SPARK-20189][DStream] Fix spark kinesis testcases to remove deprecated createStream and use Builders #17506

Uh oh!

Conversation

yssharma commented Apr 1, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yssharma commented Apr 3, 2017

Uh oh!

srowen commented Apr 3, 2017

Uh oh!

yssharma commented Apr 3, 2017

Uh oh!

srowen commented Apr 3, 2017

Uh oh!

SparkQA commented Apr 3, 2017

Uh oh!

yssharma commented Apr 3, 2017

Uh oh!

yssharma commented Apr 4, 2017

Uh oh!

srowen commented Apr 4, 2017

Uh oh!

yssharma commented Apr 4, 2017

Uh oh!

yssharma commented Apr 6, 2017

Uh oh!

srowen commented Apr 6, 2017

Uh oh!

srowen commented Apr 6, 2017

Uh oh!

SparkQA commented Apr 6, 2017

Uh oh!

yssharma commented Apr 6, 2017

Uh oh!

yssharma commented Apr 8, 2017

Uh oh!

yssharma commented Apr 12, 2017

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Apr 13, 2017

Uh oh!

yssharma commented Apr 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants