ibessonov commented on code in PR #2390:
URL: https://github.com/apache/ignite-3/pull/2390#discussion_r1298181750
##########
modules/file-transfer/src/main/java/org/apache/ignite/internal/network/file/ChunkedFileReader.java:
##########
@@ -81,17 +81,21 @@ boolean hasNextChunk() {
* file.
*
* @return Chunk data.
- * @throws IllegalStateException If there are no more chunks to read.
* @throws IOException If an I/O error occurs.
*/
byte[] readNextChunk() throws IOException {
if (!hasNextChunk()) {
- throw new IllegalStateException("No more chunks to read");
+ throw new IOException("No more chunks to read");
}
int toRead = (int) Math.min(chunkSize, length - offset);
byte[] data = new byte[toRead];
- stream.read(data);
+ int read = stream.read(data);
+
+ if (read != toRead) {
+ throw new IOException("Failed to read chunk data from file:
expected " + toRead + ", actual " + read + "]");
+ }
Review Comment:
This is not what I meant, you should read in a loop.
-1 would mean an unexpected end of file, but you don't check for it
exclusively. Please use the API properly
##########
modules/file-transfer/src/main/java/org/apache/ignite/internal/network/file/FileTransferServiceImpl.java:
##########
@@ -244,69 +254,84 @@ private void
processFileTransferInitMessage(FileTransferInitMessage message, Str
Identifier identifier = message.identifier();
- CompletableFuture<Path> directoryFuture = supplyAsync(() ->
createTransferDirectory(transferId), executorService);
+ CompletableFuture<Path> directoryFuture = supplyAsync(() ->
createTransferDirectory(transferId), executorService)
+ .whenComplete((directory, e) -> {
+ if (e != null) {
+ LOG.error("Failed to create transfer directory
[transferId={}, identifier={}]", e, transferId, identifier);
+ }
+ });
CompletableFuture<List<Path>> uploadedFiles =
directoryFuture.thenCompose(directory -> fileReceiver.registerTransfer(
senderConsistentId,
transferId,
message.headers(),
directory
- )).whenComplete((v, e) -> {
- if (e != null) {
- LOG.error("Failed to register file transfer. Transfer ID: {}.
Metadata: {}", e, transferId, identifier);
- }
- });
-
- FileTransferInitResponse response =
messageFactory.fileTransferInitResponse().build();
-
- messagingService.respond(senderConsistentId, FILE_TRANSFER_CHANNEL,
response, correlationId)
+ ));
+
+ // Check that directory was created successfully and send response to
the sender.
+ directoryFuture.handle((directory, e) -> {
Review Comment:
I know you're tired of me at this point, but does it really have to be one
huge statement in the code?
Why not split it into several statements, by extracting temporary results
into variables?
Why not extract some parts into methods?
Are you comfortable yourself with the code of such complexity?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
