-
Notifications
You must be signed in to change notification settings - Fork 144
JDK Transport: Do no longer leverage temp file for transfering artifact #1755
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
base: master
Are you sure you want to change the base?
Conversation
| assertEquals(0L, listener.getDataOffset()); | ||
| assertEquals(6L, listener.getDataLength()); | ||
| assertEquals(1, listener.getStartedCount()); | ||
| assertTrue(listener.getStartedCount() > 0 && listener.getStartedCount() <= 2, "Started count: " + listener.getStartedCount()); |
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.
There are some more tests which are strict with regard to started count. In general depending on the HTTP client it may be started multiple times (due to internal retries). Can we also relax the other assertions @cstamas?
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.
Not sure yet why they only fail with Java < 25...
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.
That was the exact reason why it used tmp file, I wanted higher level APIs (ie listeners most importantly) protect from low level stuff, and make them behave in same way (re emitted events).
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.
But that way you don't get real results, e.g. progress reported on the listener is not upload progress but copy progress to the temp file. I consider this worse than having different values for the started count. Not sure what the started count is used for potentially by consumers, but obviously retries may differ i.e. it is expected to have different numbers depending on the transport.
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.
@cstamas Are you fine with relaxing the other ITs assertions with respect to num started counts?
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.
Yup, is fine.
3ed3e8c to
5b011e1
Compare
|
Where are we with this? |
|
I need to relax some more ITs |
3c0ba78 to
ae65795
Compare
with PUT Directly transfer based on the PutTask's InputStream. Improve retry handling to not retry (some) BodyPublisher exceptions.
ae65795 to
4c7d694
Compare
with PUT
Directly transfer based on the PutTask's InputStream. Improve retry handling to not retry (some) BodyPublisher exceptions.
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.