Skip to content

Conversation

@BrianLondon
Copy link
Contributor

Successfully ran kinesis demo on a live, aws hosted kinesis stream against master and 1.6 branches. For reasons I don't entirely understand it required a manual merge to 1.5 which I did as shown here: BrianLondon@075c22e

The demo ran successfully on the 1.5 branch as well.

According to mvn dependency:tree it is still pulling a fairly old version of the aws-java-sdk (1.9.37), but this appears to have fixed the kinesis regression in 1.5.2.

@JoshRosen
Copy link
Contributor

Instead of just omitting the dependency, what about marking it as provided?

@srowen
Copy link
Member

srowen commented Dec 28, 2015

@JoshRosen it's not the scope that's an issue but the version. Not specifying it lets the SDK version required by the Kinesis client come in at whatever it needs to be. I am not sure provided scope works since it does really need to be bundled and isn't necessarily otherwise available from the env.

There's a little wrinkle here in that the Kinesis code uses SDK classes directly, so technically the POM should declare that. However the SDK is used only in the context of the Kinesis client, so it seems like the lesser evil to rely on it as a transitive dependency but at the right version.

@SparkQA
Copy link

SparkQA commented Dec 30, 2015

Test build #2270 has finished for PR 10492 at commit 8e80f5e.

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

@srowen
Copy link
Member

srowen commented Jan 2, 2016

@JoshRosen are you OK with the reasoning above?
@BrianLondon sorry but the style checker doesn't like the white space on the blank line you added to trigger tests :(. If that's removed I can merge this.

@BrianLondon
Copy link
Contributor Author

@srowen space removed. Thanks.

@JoshRosen
Copy link
Contributor

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jan 4, 2016

Test build #48668 has finished for PR 10492 at commit 137bafd.

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

@srowen
Copy link
Member

srowen commented Jan 5, 2016

Given the discussion here, I'm pretty confident in this change and would like to go ahead and merge it. It will also unblock further fixes in 12269.

@BrianLondon
Copy link
Contributor Author

Sounds good to me. Once done, I'll rebase 12269 and reopen that discussion.

@asfgit asfgit closed this in ff89975 Jan 5, 2016
asfgit pushed a commit that referenced this pull request Jan 5, 2016
Successfully ran kinesis demo on a live, aws hosted kinesis stream against master and 1.6 branches.  For reasons I don't entirely understand it required a manual merge to 1.5 which I did as shown here: BrianLondon@075c22e

The demo ran successfully on the 1.5 branch as well.

According to `mvn dependency:tree` it is still pulling a fairly old version of the aws-java-sdk (1.9.37), but this appears to have fixed the kinesis regression in 1.5.2.

Author: BrianLondon <[email protected]>

Closes #10492 from BrianLondon/remove-only.

(cherry picked from commit ff89975)
Signed-off-by: Sean Owen <[email protected]>
asfgit pushed a commit that referenced this pull request Jan 5, 2016
Successfully ran kinesis demo on a live, aws hosted kinesis stream against master and 1.6 branches.  For reasons I don't entirely understand it required a manual merge to 1.5 which I did as shown here: BrianLondon@075c22e

The demo ran successfully on the 1.5 branch as well.

According to `mvn dependency:tree` it is still pulling a fairly old version of the aws-java-sdk (1.9.37), but this appears to have fixed the kinesis regression in 1.5.2.

Author: BrianLondon <[email protected]>

Closes #10492 from BrianLondon/remove-only.

(cherry picked from commit ff89975)
Signed-off-by: Sean Owen <[email protected]>
@srowen
Copy link
Member

srowen commented Jan 5, 2016

Merged to master/1.6/1.5. I'll keep an eye on the 1.5 build to make sure that cherry pick still worked OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants