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

Add backward-compatible setMultipartCopyThreshold #637

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@romi-totango

romi-totango commented Feb 21, 2016

to allow S3A to work with Hadoop 2.7

instead of waiting for Hadoop 2.8
as described here
https://issues.apache.org/jira/browse/HADOOP-12420

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 21, 2016

it says the compilation failure is "javac: invalid target release: 1.7" when building with "openjdk6", well obviously...
but this is unrelated to my changes

romi-totango commented Feb 21, 2016

it says the compilation failure is "javac: invalid target release: 1.7" when building with "openjdk6", well obviously...
but this is unrelated to my changes

@breedloj

This comment has been minimized.

Show comment
Hide comment
@breedloj

breedloj Feb 22, 2016

Contributor

Hey Romi, thanks very much for the PR! Makes sense to introduce this in order to unblock users on existing versions of Hadoop. That said I would probably have a preference towards marking the method as deprecated so that we can eventually plan to get away from it in a future major version bump of the SDK (and once several versions of Hadoop containing hitting the new method have been proliferated throughout the user base). Aside from that my only other question is if you can please confirm you are submitting this code under the Apache 2.0 license?

Contributor

breedloj commented Feb 22, 2016

Hey Romi, thanks very much for the PR! Makes sense to introduce this in order to unblock users on existing versions of Hadoop. That said I would probably have a preference towards marking the method as deprecated so that we can eventually plan to get away from it in a future major version bump of the SDK (and once several versions of Hadoop containing hitting the new method have been proliferated throughout the user base). Aside from that my only other question is if you can please confirm you are submitting this code under the Apache 2.0 license?

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 22, 2016

hi @breedloj
i've added a more detailed comment and marked the method as deprecated.
of course my change is submitted under the Apache 2.0 license 👍.
please let me know at which SDK version can I expect to see this available.
p.s. why is the build server trying to build it with openjdk6?

romi-totango commented Feb 22, 2016

hi @breedloj
i've added a more detailed comment and marked the method as deprecated.
of course my change is submitted under the Apache 2.0 license 👍.
please let me know at which SDK version can I expect to see this available.
p.s. why is the build server trying to build it with openjdk6?

@breedloj

This comment has been minimized.

Show comment
Hide comment
@breedloj

breedloj Feb 22, 2016

Contributor

Great, thanks for the update! I've staged this change so that it'll go out as part of our next scheduled release this week.

Contributor

breedloj commented Feb 22, 2016

Great, thanks for the update! I've staged this change so that it'll go out as part of our next scheduled release this week.

@breedloj breedloj closed this Feb 22, 2016

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 23, 2016

thanks!
what do you mean staged? should the PR be merged for that?

romi-totango commented Feb 23, 2016

thanks!
what do you mean staged? should the PR be merged for that?

@breedloj

This comment has been minimized.

Show comment
Hide comment
@breedloj

breedloj Feb 23, 2016

Contributor

Our release process happens internally and out of band of GitHub. Hence we must pull in new PRs locally to stage them for our release, at which point the changes will make their way out to the public SDK artifacts. We tentatively have a release scheduled for today which this change should be a part of. I will definitely let you know once it has been released.

Contributor

breedloj commented Feb 23, 2016

Our release process happens internally and out of band of GitHub. Hence we must pull in new PRs locally to stage them for our release, at which point the changes will make their way out to the public SDK artifacts. We tentatively have a release scheduled for today which this change should be a part of. I will definitely let you know once it has been released.

@breedloj

This comment has been minimized.

Show comment
Hide comment
@breedloj

breedloj Feb 24, 2016

Contributor

Just wanted to give you an update: our release process is done and the fix is now live. Thanks again!

Contributor

breedloj commented Feb 24, 2016

Just wanted to give you an update: our release process is done and the fix is now live. Thanks again!

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 24, 2016

hi @breedloj
turns out that I overloaded the wrong method, so the patch doesn't fix the issue
here's an updated version, with the correct method, and also with a link to the original commit that broke it in v1.7.6

romi-totango@d7453a7

can you please merge this?
thanks!

romi-totango commented Feb 24, 2016

hi @breedloj
turns out that I overloaded the wrong method, so the patch doesn't fix the issue
here's an updated version, with the correct method, and also with a link to the original commit that broke it in v1.7.6

romi-totango@d7453a7

can you please merge this?
thanks!

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 24, 2016

just for reference, he's the PR that started it all
#201

romi-totango commented Feb 24, 2016

just for reference, he's the PR that started it all
#201

@breedloj

This comment has been minimized.

Show comment
Hide comment
@breedloj

breedloj Feb 24, 2016

Contributor

Hey Romi,

At this point since your original fix went out we will be unable to remove that method as users may already be using it and we cannot make breaking changes until we have a major version bump. Can you create a new PR with the updated fix on top of the current codebase?

Thanks.

Contributor

breedloj commented Feb 24, 2016

Hey Romi,

At this point since your original fix went out we will be unable to remove that method as users may already be using it and we cannot make breaking changes until we have a major version bump. Can you create a new PR with the updated fix on top of the current codebase?

Thanks.

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 24, 2016

Hi,
I overloaded a method which had a 'long' param with a method that has a 'int' param. Whoever is/was using it, will call the method with the 'long' param anyway. So I'm quite sure that nobody anywhere ever will use the new first method (setMultipartCopyThreshold(int)) that I've just added!
I merely added the wrong method, and I want to add the right one instead.

Sure, I'll make a new PR from the new code base.
Would you like me to just add the second method (setMultipartUploadThreshold), and not remove the one which was added by mistake (setMultipartCopyThreshold) and already went into the release?

romi-totango commented Feb 24, 2016

Hi,
I overloaded a method which had a 'long' param with a method that has a 'int' param. Whoever is/was using it, will call the method with the 'long' param anyway. So I'm quite sure that nobody anywhere ever will use the new first method (setMultipartCopyThreshold(int)) that I've just added!
I merely added the wrong method, and I want to add the right one instead.

Sure, I'll make a new PR from the new code base.
Would you like me to just add the second method (setMultipartUploadThreshold), and not remove the one which was added by mistake (setMultipartCopyThreshold) and already went into the release?

@breedloj

This comment has been minimized.

Show comment
Hide comment
@breedloj

breedloj Feb 24, 2016

Contributor

Yeah, I spoke with the team about this as well and we believe it'd be safe to remove the erroneous overload so I'd go ahead and do that in the new PR.

Contributor

breedloj commented Feb 24, 2016

Yeah, I spoke with the team about this as well and we believe it'd be safe to remove the erroneous overload so I'd go ahead and do that in the new PR.

@romi-totango

This comment has been minimized.

Show comment
Hide comment
@romi-totango

romi-totango Feb 25, 2016

created a new PR
#643

romi-totango commented Feb 25, 2016

created a new PR
#643

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