Skip to content

Conversation

balamurugana
Copy link
Member

@balamurugana balamurugana commented Feb 4, 2025

  • Move dependent classes as subclasses in messages
  • PutObject supports parallel uploads and checksum
  • Examples are updated
  • Dependencies are upgraded
  • Remove all deprecated methods

@balamurugana balamurugana marked this pull request as draft February 4, 2025 12:09
@balamurugana balamurugana force-pushed the Refactor-messages-by-subclasses branch 27 times, most recently from c7bd699 to e48331a Compare February 17, 2025 13:00
@balamurugana balamurugana force-pushed the Refactor-messages-by-subclasses branch 2 times, most recently from 9f9d9fa to d236c38 Compare February 18, 2025 13:43
@harshavardhana
Copy link
Member

@balamurugana is this ready to be reviewed?

@balamurugana
Copy link
Member Author

@harshavardhana

@balamurugana is this ready to be reviewed?
This PR is complete. As it a breaking change, we would need to make next v8.5.x release, so that it fixes in the v8.5.x

@balamurugana balamurugana force-pushed the Refactor-messages-by-subclasses branch from 1eeb6e7 to 330157e Compare August 26, 2025 00:16
@balamurugana balamurugana force-pushed the Refactor-messages-by-subclasses branch 3 times, most recently from b20bf98 to bc2d471 Compare September 19, 2025 06:41
@balamurugana balamurugana force-pushed the Refactor-messages-by-subclasses branch 8 times, most recently from 0afc13f to e064a53 Compare September 28, 2025 08:11
@balamurugana balamurugana marked this pull request as ready for review September 28, 2025 08:58
@nagashreem
Copy link

@harshavardhana

@balamurugana is this ready to be reviewed?
This PR is complete. As it a breaking change, we would need to make next v8.5.x release, so that it fixes in the v8.5.x

@harshavardhana is the release done? @balamurugana is there anything else pending here other than the reviews?
@Praveenrajmani @kanagarajkm can we please get some reviews here?

Comment on lines +261 to +265
} catch (EOFException e) {
throw new MinioException(e);
} catch (IOException e) {
throw new MinioException(e);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (EOFException e) {
throw new MinioException(e);
} catch (IOException e) {
throw new MinioException(e);
}
} catch (EOFException | IOException e) {
throw new MinioException(e);
}

if not using specific custom message?

Comment on lines +327 to +331
} catch (EOFException e) {
throw new MinioException(e);
} catch (IOException e) {
throw new MinioException(e);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (EOFException e) {
throw new MinioException(e);
} catch (IOException e) {
throw new MinioException(e);
}
} catch (EOFException | IOException e) {
throw new MinioException(e);
}

Comment on lines +105 to +109
@JsonProperty("objectsReplicatedTotalSize")
private long objectsReplicatedTotalSize;

@JsonProperty("objectReplicaTotalSize")
private long objectReplicaTotalSize;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are these two fields different?

Comment on lines +360 to +378
private BigDecimal totalspace;

@JsonProperty("usedspace")
private BigDecimal usedspace;

@JsonProperty("availspace")
private BigDecimal availspace;

@JsonProperty("readthroughput")
private BigDecimal readthroughput;

@JsonProperty("writethroughput")
private BigDecimal writethroughput;

@JsonProperty("readlatency")
private BigDecimal readlatency;

@JsonProperty("writelatency")
private BigDecimal writelatency;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no camelCase for naming?

@Nullable String group)
throws MinioException {
if (!(user != null ^ group != null)) {
throw new IllegalArgumentException("either user or group must be provided");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be throwing MinioException here?

Comment on lines +846 to 850
} catch (JsonProcessingException e) {
throw new MinioException(e);
} catch (IOException e) {
throw new MinioException(e);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (JsonProcessingException e) {
throw new MinioException(e);
} catch (IOException e) {
throw new MinioException(e);
}
} catch (JsonProcessingException | IOException e) {
throw new MinioException(e);
}

Comment on lines +73 to +98
// Inherited by MinioException
if (e instanceof BucketPolicyTooLargeException) throw (BucketPolicyTooLargeException) e;
if (e instanceof ErrorResponseException) throw (ErrorResponseException) e;
if (e instanceof InsufficientDataException) throw (InsufficientDataException) e;
if (e instanceof InternalException) throw (InternalException) e;
if (e instanceof InvalidResponseException) throw (InvalidResponseException) e;
if (e instanceof ServerException) throw (ServerException) e;
if (e instanceof XmlParserException) throw (XmlParserException) e;

// Inherited by IOException
if (e instanceof JsonMappingException) throw (JsonMappingException) e;
if (e instanceof JsonParseException) throw (JsonParseException) e;
if (e instanceof JsonProcessingException) throw (JsonProcessingException) e;
if (e instanceof EOFException) throw (EOFException) e;
if (e instanceof FileNotFoundException) throw (FileNotFoundException) e;
if (e instanceof IOException) throw (IOException) e;

// Inherited by GeneralSecurityException
if (e instanceof CertificateException) throw (CertificateException) e;
if (e instanceof InvalidKeyException) throw (InvalidKeyException) e;
if (e instanceof KeyManagementException) throw (KeyManagementException) e;
if (e instanceof KeyStoreException) throw (KeyStoreException) e;
if (e instanceof NoSuchAlgorithmException) throw (NoSuchAlgorithmException) e;
if (e instanceof GeneralSecurityException) throw (GeneralSecurityException) e;

throw this;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this, when

  • All these exceptions already extend their respective parent classes
  • They will naturally propagate up the call stack
  • No need to manually check and re-throw

private long size;
private ZonedDateTime lastModified;
private RetentionMode retentionMode;
private ZonedDateTime retentionRetainUntilDate;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private ZonedDateTime retentionRetainUntilDate;
private ZonedDateTime retainUntilDate;

should be good enough name

@balamurugana
Copy link
Member Author

@harshavardhana

@balamurugana is this ready to be reviewed?
This PR is complete. As it a breaking change, we would need to make next v8.5.x release, so that it fixes in the v8.5.x

@harshavardhana is the release done? @balamurugana is there anything else pending here other than the reviews? @Praveenrajmani @kanagarajkm can we please get some reviews here?

@nagashreem Other than review, nothing is pending

* Move dependent classes as subclasses in messages
* PutObject supports parallel uploads and checksum
* Examples are updated
* Dependencies are upgraded
* Remove all deprecated methods
* Add appendObject API

Signed-off-by: Bala.FA <bala@minio.io>
@balamurugana balamurugana force-pushed the Refactor-messages-by-subclasses branch from e064a53 to 93cfd5b Compare October 11, 2025 04:39
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