IGNITE-19009 Introduce file transfer support in messaging #2390

ivgag · 2023-08-01T10:43:10Z

https://issues.apache.org/jira/browse/IGNITE-19009

.../testFixtures/java/org/apache/ignite/internal/testframework/matchers/FileContentMatcher.java

modules/file-transferring/build.gradle

...ansferring/src/testFixtures/java/org/apache/ignite/internal/network/file/FileAssertions.java

...src/integrationTest/java/org/apache/ignite/internal/network/file/ItFileTransferringTest.java

...es/file-transferring/src/main/java/org/apache/ignite/internal/network/file/FileReceiver.java

...transferring/src/main/java/org/apache/ignite/internal/network/file/messages/ChunkedFile.java

modules/network-api/src/main/java/org/apache/ignite/network/MessagingService.java

…entEquals

...fer/src/integrationTest/java/org/apache/ignite/internal/network/file/ItFileTransferTest.java

...s/file-transfer/src/main/java/org/apache/ignite/internal/network/file/ChunkedFileReader.java

ibessonov · 2023-08-16T09:43:31Z

...s/file-transfer/src/main/java/org/apache/ignite/internal/network/file/ChunkedFileReader.java

-        return raf.length();
+    int nextChunkNumber() {
+        if (!hasNextChunk()) {
+            throw new IllegalStateException("No more chunks to read");


Same here, this is one of the most widely misused exceptions out there.
Why do we even need the exception in this method in the first place?

I would expect that there is a next chunk if it has a number. So we have to return a special value (e.g., -1) or throw an exception to indicate that there are no more chunks to read. Actually, I don't see a difference between those approaches because it is expected that a user has checked the absence of the next chunk by calling hasNextChunk().

...s/file-transfer/src/main/java/org/apache/ignite/internal/network/file/ChunkedFileWriter.java

...-transfer/src/main/java/org/apache/ignite/internal/network/file/FileTransferServiceImpl.java

ibessonov · 2023-08-16T10:04:54Z

...-transfer/src/main/java/org/apache/ignite/internal/network/file/FileTransferServiceImpl.java

            FileConsumer<M> consumer
    ) {
-        metadataToHandler.compute(
+        metadataToConsumer.compute(


Why did you renamed "handler" to "consumer"? Consumer is too broad of a term

ibessonov · 2023-08-16T10:06:28Z

...-transfer/src/main/java/org/apache/ignite/internal/network/file/FileTransferServiceImpl.java

+            List<Path> paths
+    ) {
+        if (paths.isEmpty()) {
+            return failedFuture(new FileTransferException("No files to upload"));


Why is this a problem? What happens if user tries to do this, will he have an error in his log? It's not what he would expect

I find it puzzling when someone uploads or downloads an empty list of files. What could this mean? That the files weren't found, or something else?

I would prefer to keep empty lists restricted to avoid confusion until we really need support for this. Right now we are going to transfer only deployment units. It's impossible to have a deployment unit without any files.

What error will a user see in this case? If it's an error message in a server log, then it's a problem. If it's an exception message in his application, then I'm fine with it

modules/file-transfer/src/test/java/org/apache/ignite/internal/network/file/FileSenderTest.java

ibessonov · 2023-08-18T08:41:41Z

...s/file-transfer/src/main/java/org/apache/ignite/internal/network/file/ChunkedFileReader.java

+        if (read != toRead) {
+            throw new IOException("Failed to read chunk data from file: expected " + toRead + ", actual " + read + "]");
+        }


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

ibessonov · 2023-08-18T08:45:09Z

...-transfer/src/main/java/org/apache/ignite/internal/network/file/FileTransferServiceImpl.java

+        ));
+
+        // Check that directory was created successfully and send response to the sender.
+        directoryFuture.handle((directory, e) -> {


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?

ibessonov · 2023-08-18T12:29:48Z

...s/file-transfer/src/main/java/org/apache/ignite/internal/network/file/ChunkedFileReader.java

+        hasNextChunk = stream.available() > 0;

-        if (offset == length) {
+        if (stream.available() == 0) {


I wonder why it's not a !hasNextChunk, but whatever.
I've been looking at this code for too long. If there are more problems, I probably won't notice them at this point

…st.transferIsRegisteredBeforeResponseIsSent

ivgag changed the title ~~IGNITE-19009 WIP~~ IGNITE-19009 Introduce file transfer support in messaging Aug 1, 2023

ivgag force-pushed the IGNITE-19009 branch 13 times, most recently from a5bb82f to 187fe66 Compare August 2, 2023 06:15

IGNITE-19009 WIP

8bb14dd

ivgag force-pushed the IGNITE-19009 branch from 187fe66 to 8bb14dd Compare August 2, 2023 06:23

rpuch reviewed Aug 2, 2023

View reviewed changes

ivgag added 13 commits August 2, 2023 16:50

fix matcher

85231ff

rename module

6d8f91b

fix org.apache.ignite.internal.network.file.FileAssertions.assertCont…

993f5c1

…entEquals

fix comments

bf3d857

write files in the separated threads

f6b0b88

create file transferring configuration

0cebde5

improve error handling

a0e5491

refactor FileSender

2017a47

change api

753f3ba

rename TransferMetadata to Metadata

fe5515a

restrict empty files list

4b4833b

add test when directory is not writable

99e284c

fix FileTransferMessagesHandler

011c0a4

ivgag added 6 commits August 14, 2023 19:45

rework download mechanism

0b28edd

rework download mechanism

cf7aea2

fix checkstyle

a546e88

Merge remote-tracking branch 'upstream/main' into IGNITE-19009

82c548f

refactor ChunkedFileWriter

d8c6392

refactor ChunkedFileWriter

88d2dd7

ibessonov reviewed Aug 16, 2023

View reviewed changes

ivgag added 11 commits August 16, 2023 22:58

fix comments

fa0abe6

update README.md

6448ec5

fix checkstyle

c8e77fe

fix exception messages

2d73dc9

fix exception messages

f28470e

fix exception messages

4b42d71

fix exception messages

0c17431

update javadoc

ae981eb

small refactor

90566b7

small refactor

a60c9c9

small refactoring

a6bbd13

ibessonov reviewed Aug 18, 2023

View reviewed changes

fix comments

6b62855

ibessonov reviewed Aug 18, 2023

View reviewed changes

ibessonov approved these changes Aug 18, 2023

View reviewed changes

ivgag added 6 commits August 18, 2023 23:45

fix comments

b621428

fix tests

392d813

Merge remote-tracking branch 'upstream/main' into IGNITE-19009

933273c

fix FileTransferServiceImpl

c493856

fix FileTransferServiceImpl

5952011

fix org.apache.ignite.internal.network.file.FileTransferServiceImplTe…

e4a33c3

…st.transferIsRegisteredBeforeResponseIsSent

PakhomovAlexander merged commit 77d9823 into apache:main Aug 21, 2023

ivgag deleted the IGNITE-19009 branch August 21, 2023 12:13

IGNITE-19009 Introduce file transfer support in messaging #2390

IGNITE-19009 Introduce file transfer support in messaging #2390

Uh oh!

Conversation

ivgag commented Aug 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants