Separate protocol related methods into own class about iris-java

[paplorinc]

... or could be composed using inheritance.
However, I vote for separation in layers (e.g. request-response, broadcast etc. in separate classes that inherit from a generic Connection class)

[evfool]

+1 for functionality-based separation of classes, each inheriting from the Connection class containing the methods for communication like sending and receiving primitive data types (ported from proto.go)

[evfool]

-1 for functionality-based separation: in real-life, nodes/applications might want to do multiple operations, e.g. broadcast a message to each other node, but also send a private message in form of a request/reply to another node. If we separate broadcast in a class, and request-reply in another, such a client will have to have two separate classes (the class for Broadcast with only a single method, which is not the best design) to interact with, and if both inherit from the same Connection, both might open a Socket, thus unnecessarily consuming resources.

[paplorinc]

Yes, but you could compose those classes also, instead of inheritance e.g. by giving it a Connection instance, like:

Connection connection = ...

RequestResponceOperation requestResponce = new RequestResponce(connection);
requestResponce.sendRequest(clusterName, request, timeout);

BroadcastOperation broadcast = new BroadcastOperation(connection);
broadcast.send(clusterName, message);

The usage might be more complicated like this (though in real-life I guess it would be about the same), but at least the functionality would be separated.

[karalabe]

Please note, that a lot of effort went into the API design of the Go binding, and that is the reference implementation. The design choices were not arbitrary, but a result of about a month's time of constant refactor. Please try to understand the logic behind that code before suggesting new designs.

The proposed design of

RequestResponceOperation requestResponce = new RequestResponce(connection);
requestResponce.sendRequest(clusterName, request, timeout);

opposed to the existing

connection.request(cluster, message, timeout)

is an extreme over complication that effectively wraps single function call into a whole class. That is a very serious overhead that is not warranted.

As an internal implementation it could be disputed whether separation of communication primitives is a good idea or bad, but I say it is a bad one. The reason is that in network programming and protocols, you have to think in layers. Everything that belongs to a specific layer must coexist beside each other so that each layer can be modified independently of all other layers. You always build from the ground up, each individual layer relying on the one below.

Splitting vertically functionality wise would result in each and every such class containing a lot of overlapping logic. Yes, inheritance could solve some issues, but a significant amount of new issues arise (e.g. who will read the socket, and how will that reader know to whom to forward the result?). You simply cannot remove the demultiplexing logic from the connection, and that logic will need to keep track of all pending operations (i.e. you won't save anything by trying to separate logic, because they are too tightly tied together).

The protocol itself is simple, albeit yes, it can result in a sizable connection class. IMHO, that is not an issue. I'm not really prepared to sacrifice API and design simplicity just to keep the size of a file down. Separating the wire protocol serialization and deserialization may be worthwhile, although that already litters the API caused simply by Java not allowing split classes.

I'm open to debate on the reasoning above, but in my opinion the correct way to proceed in the current pristine state of the binding is to start with the Go version that works correctly, is blazing fast and beautiful in itself, and get that ported over to Java, making minor language adjustments. Then, after all the logic is in place, it can be argued what could be and what should be split. You'll find that the existing design is quite solid and yet trivially simple. I'd like to keep it that way.

[paplorinc]

is an extreme over complication that effectively wraps single function call into a whole class. That is a very serious overhead that is not warranted.

Isn't the difference between object oriented and procedural design, that functions are separated into coherent classes, instead of putting all the somewhat related classes to one God entity, saying that they are inseparable and belong all together?

I doubt that my proposed solution would be extreme overcomplication with a very serious overhead (let's not get carried away), and it's not a bad practice to put a single method inside a class (actually, SAMs are the building blocks of Java 8 lambdas).

I also doubt that this would result in a measurable overhead (especially after JVM warmup, which also eliminates chained references where necessary), but I'm sure it would make the API more readable and more SOLID (i.e. Single Responsibility, Open/Closed and especially the Interface Segregation part). Also, this way the callback interfaces could also be segregated (e.g. ConnectionHandler), instead of providing a general interface, where most methods could return an exception, if they were called in a wrong state.

I'm not trying to say that this would certainly be better than any other solution (unlike what I feel is the case for the current implementation), I'm just suggesting openness for this alternative, which should be investigated once the application is working correctly.

litters the API caused simply by Java not allowing split classes

Sure it does: by inheritance. I'm glad it doesn't allow split classes, inheritance already violates encapsulation.

Splitting vertically functionality wise would result in each and every such class containing a lot of overlapping logic.

Only if you design it that way on purpose.

they are too tightly tied together

Exactly, that's my main concern also.
In theory you could have a request/response messaging without broadcast. We shouldn't couple functionality that doesn't strictly belong together (i.e. can't exist without the other).
In my opinion this would make the API cleaner, on the contrary to sacrificing API and design simplicity.

[karalabe]

"A programmer will be able to understand a problem in full only when he has solved it at least once" ~Francesco Cesarini

I'm not trying to to rule out alternative designs. I'm trying to underline that the existing design is the result of a lengthy process - with actual real world use - whereas the proposed design is only at an idea level. It is very hard to argue against ideas.

If you'd be willing to put together an alternative API proposal (only the public API, the internals aren't important now), that is fully expanded on all features (i.e. discusses both inbound and outbound messages as well as the tunnels), I'd be willing to consider it and/or debate it :)

[paplorinc]

That's what I proposed :).
When the API is functional (and backed by tests) we should investigate the alternative together, taking into consideration:

• logical coherence, ease of comprehension/use and design simplicity;
• extensibility (even if we can't see exactly how it could be changed in the future);
• running speed comparison.

[paplorinc]

Decoupled the connection class from the protocol implementations, delegated many inheritances, etc.
Many things are still wrong, but will tackle those later.