-
Notifications
You must be signed in to change notification settings - Fork 485
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
Java Docs Update #730
Java Docs Update #730
Conversation
* DeleteError error = errorResult.get(); | ||
* System.out.println("Failed to remove '" + error.objectName() + "'. Error:" + error.message()); | ||
* } }</pre> | ||
* | ||
* @param bucketName Bucket name. | ||
* @param objectNames List of Object names in the bucket. | ||
*/ | ||
public Iterable<Result<DeleteError>> removeObject(final String bucketName, final Iterable<String> objectNames) { | ||
public Iterable<Result<DeleteError>> removeObjects(final String bucketName, final Iterable<String> objectNames) { |
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.
Is it agreed to change the name? If so, its a breaking change and you would need to bump up major version.
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.
Is it agreed to change the name?
Checked with @kannappanr @deekoder before changing the name.
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.
Only the public method name is changed. Made the changed in minio-java
. Please tell what can break because of this? I can retrofit it in the PR.
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.
Currently available http://minio.github.io/minio-java/io/minio/MinioClient.html#removeObject-java.lang.String-java.lang.Iterable- is removed and new method removeObjects()
is available to do that same in next release. Already developed code of users using http://minio.github.io/minio-java/io/minio/MinioClient.html#removeObject-java.lang.String-java.lang.Iterable- will get an error for next release. This needs to be notified indirectly by bumping up major version.
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.
We did discuss the impact of this breaking change with AB. I think we just have to call it out as a BREAKING CHANGE when we do a release with this PR.
@@ -3413,7 +3413,7 @@ private String putObject(String bucketName, String objectName, int length, | |||
*/ | |||
private void putObject(String bucketName, String objectName, Long size, Object data, | |||
Map<String, String> headerMap, ServerSideEncryption sse) | |||
throws InvalidBucketNameException, NoSuchAlgorithmException, InsufficientDataException, IOException, | |||
throws InvalidBucketNameException, NoSuchAlgorithmException, IOException, |
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.
Isn't UnsufficentDataException
going to be thrown any more for all the above methods?
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.
InsufficientDataException
was redundant in the signature, It was coming twice. So removed the extra one.
cce7b64
to
877e42b
Compare
| | ``ErrorResponseException`` : upon unsuccessful execution. | | ||
| | ``InternalException`` : upon internal library error. | | ||
| | ``InvalidArgumentException`` : upon invalid value is passed to a method. | | ||
| | ``InsufficientDataException`` : Thrown to indicate that reading given InputStream gets EOFException before reading given length. | |
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.
InsufficientDataException
is removed up at the top.
Do we still throw this exception?
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.
InsufficientDataException is still there in the method signature. I was just that InsufficientDataException was defined twice in the method signature. I removed one just to remove the redundancy.
| | ``ErrorResponseException`` : upon unsuccessful execution. | | ||
| | ``InternalException`` : upon internal library error. | | ||
| | ``InvalidArgumentException`` : upon invalid value is passed to a method. | | ||
| | ``InsufficientDataException`` : Thrown to indicate that reading given InputStream gets EOFException before reading given length. | |
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.
InsufficientDataException
is removed up at the top.
Do we still throw this exception?
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.
InsufficientDataException
is still there in the method signature. I was just that InsufficientDataException
was defined twice in the method signature. I removed one just to remove the redundancy.
| | ``org.xmlpull.v1.XmlPullParserException`` : upon parsing response XML. | | ||
| | ``ErrorResponseException`` : upon unsuccessful execution. | | ||
| | ``InternalException`` : upon internal library error. | | ||
|
||
| | ``InvalidArgumentException`` : upon invalid value is passed to a method. | | ||
| | ``InsufficientDataException`` : Thrown to indicate that reading given InputStream gets EOFException before reading given length. | |
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.
InsufficientDataException
is removed up at the top.
Do we still throw this exception?
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.
InsufficientDataException is still there in the method signature. I was just that InsufficientDataException was defined twice in the method signature. I removed one just to remove the redundancy.
* DeleteError error = errorResult.get(); | ||
* System.out.println("Failed to remove '" + error.objectName() + "'. Error:" + error.message()); | ||
* } }</pre> | ||
* | ||
* @param bucketName Bucket name. | ||
* @param objectNames List of Object names in the bucket. | ||
*/ | ||
public Iterable<Result<DeleteError>> removeObject(final String bucketName, final Iterable<String> objectNames) { | ||
public Iterable<Result<DeleteError>> removeObjects(final String bucketName, final Iterable<String> objectNames) { |
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.
All other SDKs, Go, JavaScript, Python, .NET, have 2 methods to remove objects:
removeObject()
for single object removalremoveObjects()
for multiple object removal
I think we need to stick with that approach for consistency purposes.
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.
This is under discussion . @deekoder can you please confirm on this.
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.
By the way, no matter which way we choose to go about renaming the method, Bala's point about breaking compatibility is going to be still valid.
877e42b
to
d6a8414
Compare
I understand it is discussed with @abperiasamy now. @sinhaashish can you update on the conclusion |
Results:
|
4fded8f
to
3e01a79
Compare
5f84e4e
to
b38cde7
Compare
@balamurugana @ebozduman PTAL |
f39f98f
to
cae5a76
Compare
1b9cd51
to
fa8c0ae
Compare
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.
LGTM
de86d0b
to
ff7ea89
Compare
Java Docs been updated with adding the relevant exceptions, removing the deprecated code.
ff7ea89
to
b9fa068
Compare
ping @balamurugana @ebozduman Can you please review again? |
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.
LGTM
In java, There is error when I call line 2nd-> |
@yashchavan2001 please create a new issue with details like SDK version etc |
API.md
is updated with methods present inhttp://minio.github.io/minio-java/