[POM-leading] Capability proxy - #1200
toinehartman wants to merge 18 commits into
Conversation
8c60749 to
2c24a98
Compare
…ltipleClientProxy` (dynamic registration of capabilities)
|
DavyLandman
left a comment
DavyLandman
left a comment
There was a problem hiding this comment.
Looks good, I have some small clarififcation questions and a few improvements to the code.
| * The inner map is keyed by registration options, with a list of registrations with those exact options. The first registration in this list is always registered with the actual client, while the others are kept for internal administration. | ||
| */ | ||
| private final Map<String, CompletableFuture<Map<Object, List<Registration>>>> registrations = new ConcurrentHashMap<>(); | ||
| private final CapabilityRegistry capabilityRegistry; |
There was a problem hiding this comment.
nit: class & field have exact the same name? so capabilties or registry is verbose enough?
| * details. | ||
| * </p> | ||
| */ | ||
| class CapabilityRegistry { |
There was a problem hiding this comment.
can this not be a private class?
| * | ||
| * @param R Result type of tasks | ||
| */ | ||
| class Scheduler<R> { |
There was a problem hiding this comment.
I think all these classes (also the helper one) should become private static classes, such that no external class will be tempted to use them.
| * | ||
| * @param R Result type of tasks | ||
| */ | ||
| class Scheduler<R> { |
There was a problem hiding this comment.
Can rename this to be more specific than schedular? Maybe: MonotonicSchedular or FairSingleThreadExecutor?
Also, can we document why Executors.newSingleThreadExecutor() is not the right fit?
| var toClient = new Registration(UUID.randomUUID().toString(), method, options); | ||
| forwardRegistration(toClient).whenCompleteAsync((v, t) -> { |
There was a problem hiding this comment.
nit: how about we make forwardRegistration take the method and options as parameters and it returns the Registration it just created, such that the v in the completable future can be used by the whenComplete.
| if (fromServer == null) { | ||
| var t = new IllegalStateException("Cannot unregister a capability for which no registration was sent by a server"); | ||
| logger.trace("Unregister capability {} ({}). Failed: {}", method, id, t); | ||
| result.completeExceptionally(t); |
There was a problem hiding this comment.
is that truly a failure? and not a lost race that is fine?
like if this happens, do we need to debug a problem?
| var id = u.getId(); | ||
|
|
||
| logger.trace("Unregister capability {} ({}): Submitting to scheduler...", method, id); | ||
| return scheduler.submit(result -> { |
There was a problem hiding this comment.
after reading this code and how a user needs to think about calling result. before exiting, I'm wondering if maybe the signature should change that the closure should return a completable future? like the arguments is the scheduler and the result is a future? that way no exit out of these functions leads to a hanging future.
| // Don't unset `busy` yet. Instead, doing so is the responsibility of the closure on the previous lines | ||
| // and should happen only when the task has signaled its completion. | ||
| } else { | ||
| busy.set(false); |
There was a problem hiding this comment.
Can we document in what case can we have a scheduled attemptStartTask and still have an empty task queu?
| var result = new CompletableFuture<R>(); | ||
| var task = new Task<>(action, result); | ||
| tasks.offer(task); |
There was a problem hiding this comment.
nit: these can be inlined, and the future might be created by the task?
| } | ||
|
|
||
| public CompletableFuture<R> submit(Consumer<CompletableFuture<R>> action) { | ||
| var result = new CompletableFuture<R>(); |
There was a problem hiding this comment.
there is a subtle bug where a future should be connected to an executure, else any next element in the chain won't happen on the executor, but on the global pool.
we've made some classes that work around this issue, maybe those are nice to use?
although if we rewrite this to always return a closure, than we get rid of that problem in this specific class.
3 participants
Improve maintainability and thread-safety of the client router, specifically operations related to (un)registering dynamic capabilities.
Also see: