project-iris / iris-java Goto Github PK

Read, fill, sing :) http://iris.karalabe.com/icla

• @evfool
• @paplorinc

Maybe will need a custom logger as in Erlang. Should definitely check out the capabilities of a few java logger packages.

Specifically:

• Concurrent broadcasts
• Broadcast thread limitation
• Broadcast memory limitation
• Concurrent requests
• Failing requests (returning errors through Iris)
• Request timeout
• Request thread limitation
• Request memory limitation
• Request expiration (within local queue)
• Concurrent publishes
• Publish thread limitation
• Publish memory limitation
• Concurrent tunnels
• Tunnel timeout
• Tunnel data chunking
• Tunnel data overloading

After a successful handshake, the connection should start up a new thread that keeps reading the network socket until it is closed down (either fails, or a close notification arrives from the Iris node).

There is a tunnel race probably in the unimplemented shutdown code. The concurrent tunnel test sometimes hangs. Interestingly enough, I believe it was introduced by the allowance throttler. Maybe a lock doesn't get released or something like that when interrupted? Dunno, should be sorted out.

Since java does not have a mechanism to wait until a specific point in time, only a duration, currently during chunked sending each chunk in effect resets the timer. This needs to be solved via either recalculating the sleep duration after every chunk (easy) or by using some timer (fancy, but probably too complicated given the needed precision).

Replace it with a lambda wrapped factory method.

i.e. Function <TunnelBridge, Tunnel>

I've no clue why, but if I wrap the socket output stream into a buffered one, the request tests sometimes hang.

It would look to me as it the buffer's not flushed (cannot imagine any other reason/difference), yet... strange.

Currently all the enqueueing log messages (broadcast, request and publish) are missing a few contextual variables that the Go and Erlang ones have. Notably concerning the current capacity/usage or the buffer and infos related to the request expiration. These should be fined eventually.

This would require first the relay kill switch implementation. See project-iris/iris#51

I've noticed the bug particularly in the tunnel receive, but it may as well be present elsewhere too.

Essentially, if while blocking for an outside event on a wait, both the event arrives as well as the connection is torn down (i.e. last message of a tunnel, after which it is closed), then Java will happily exit the wait successfully, but the threads interrupted-flag remains set, so the next sleep/wait/join will immediately cause an exception.

One solution would be to always clear the interrupted flag after a notify is received, ensuring racy conditions are handled. Of course, this is if teh JVM always prioritizes notifies. If it sometimes throws the exception, then additional code is needed at that point too to check for notification and succeed if such cases.

Not that it would be very meaningful, the current silent fail is not really a happy path.

While benchmarking the requests, the system scales linearly until 16 threads, above which (32/64/128) it starts slowing down significantly to half the speed of a single threaded code. Obviously there is a contention somewhere, but I haven't managed to locate it yet. Broadcasting on 128 threads works correctly, so the contention should be somewhere within the request or response handling. Should profile this at a certain point.

As with the other bindings too, the Java version needs tests too to check if a dropped or closed connection interrupts all pending operations (tunnels for sure are not done properly at the moment).

The final missing piece from the API is the quality of service limits. Add these according to https://github.com/project-iris/iris-go/blob/relay-v1.0-draft2/limits.go

There is probably a bug somewhere or something's not done quite right yet, as the concurrent broadcast test takes 5-6 times as much as on the Go counterpart. Considering, that graceful close is not implemented yet (which requires additional messages and overhead), there's a serious lag somewhere.

Currently the client connection and service registration uses simple factory methods:

Connection conn = Iris.connect(...);
Service serv = Iris.register(...);

These look kinda weird in Java code, since most classes are created simply like:

Connection conn = new Connection(...)
Service serv = new Service(...)

Although the first version contains a tad more information, it feels somehow less idiomatic.

The high level API should conform to http://godoc.org/github.com/project-iris/iris-go as much as possible.

Additionally, the high level tests should implement more or less https://github.com/project-iris/iris-go/tree/master/tests

I'm not really happy about using the rmi.RemoteException and neither the concurrency.TimeoutException. Although name-wise they are ok, the containing package is very misleading as to the cause of error, so I'd rather have my own set of exceptions all extending IOException (since all are data transfer related).

Obviously the InterruptedException can stay as is.

Since Java does not support splitting classes into multiple files, the wire protocol related things should be separated out into its own class.

Currently some threads leak out from the closing procedure (tunnels definitely, others maybe), which prevent the JVM from shutting down after finishing the main thread.

Either put it into a different namespace or as a member class of connection. It's fairly simple so I wouldn't pollute the namespace unnecessarily with it.

Currently whenever a tunnel data transfer arrives, the return data allowance is sent on a new thread. Given that Java uses OS threads instead of soft threads (as in Go or Erlang), these are probably big performance hits. A thread pool with a handful of threads only responsible for returning the allowances should solve this issue fairly simply.

The API must follow the Go version to the letter. That means all names, parameters, types, orders, etc. (Obviously unless something's not directly portable into Java, which isn't the case hete).

http://godoc.org/gopkg.in/project-iris/iris-go.v1

Inbound events (broadcasts, requests and topic events) should not be called directly by the network receiver thread (#8), but executed on new threads. For each event type listed above, there should be a separate thread pool (with the limitation enforcement from #7).

I see you introduced a default charset for the strings. I don't really see the point of making it public. The public API uses strings for addressing (that it will serialize internally using UTF-8), and the messages are binary blobs, that the user fills as he sees fit. Iris doesn't care about the contents of the blobs, and beside testing I doubt anyone will use textual messages (protobufs are the most probable formats).

If it helps during testing, you could add a constant to the tester classes, but don't litter the public API. :)

I'm documenting and polishing up the relay protocol, particularly fixing a few issues that have been discovered since it went public back in August.

In order not to break existing client code until it is finished, both Iris and the existing client libraries contain a new git branch called relay-v1.0-draft2.

Since the Java binding is just being written, I'd suggest implementing the new draft, saving a rewrite later. The documentation for the relay protocol can be found here. I'll try and update it whenever a significant part is done. Also I'll post an update to this thread.

Since I do not want to waste time building for all the platforms after each commit, you might want to consider setting up an Go/Iris build environment: install the Go compiler and tools and configure a project path ($GOPATH).

Afterwards, download Iris, switch to the updated protocol branch and run it:

go get github.com/project-iris/iris
cd $GOPATH/github.com/project-iris/iris
git checkout relay-v1.0-draft2
go run main.go -dev

The current modifications that were made are:

• Add a DENY operation code, so the relay can deny a handshake.
• Add magic strings to the handshake to filter accidental non-Iris connections.
• The relay can either accept or deny a handshake (protocol version mismatch).

I'll keep adding to the above list, so don't close this thread :) Additionally, you might want to open new issues for discussing individual points, to keep this thread focused on the list itself.

Whenever an inbound event is scheduled for execution, it first should be checked whether the total memory consumption of the queued messages doesn't exceed a threshold value. If they do, the inbound event should be dropped.

The validators are currently based on regular expressions for checking cluster/topic names. These would seem to me to be quite performance hogs, but maybe the networking latency outweighs this impact. It's worth an experiment after writing the benchmarks.

Essentially in almost every ServiceHandler implementation that I've wrote, the only thing init did was to store the connection parameter into the handler. This approach originated from the Go and Erlang implementations which do not support the concept of abstract classes and inheritance.

In Java however, the connection could be simply injected into the base interface/class and a lot of superfluous boilerplate code saved as such. Given that the cost is negligible, but the saving code clarity wise can be substantial, I'd reckon the interface should be converted into an abstract base class.

An added benefit would be that further fields can be added (logger comes to mind) without needing to pass it as arguments to the callback methods.

Specifically:

• recvByte
• recvBool
• recvVarint
• recvBinary
• recvString