upgrade to rlp_03 version 8 #9
Conversation
…threaded executor that servers connection -> launches more connections within the same eventloop while handling event does not work because: the one launching more blocks the subsequent ones, perhaps future.whenComplete() ?
…ssageParser); too
StrongestNumber9
left a comment
StrongestNumber9
left a comment
There was a problem hiding this comment.
Added bunch of comments and change requests
| <version>3.12.1</version> | ||
| <configuration> | ||
| <compilerArgument>-Xlint:all</compilerArgument> | ||
| <source>9</source> |
There was a problem hiding this comment.
Version 9? We can enforce java8 with enforcer rules from project-archetype
| <configuration> | ||
| <parallel>all</parallel> | ||
| <useUnlimitedThreads>true</useUnlimitedThreads> | ||
| <parallel>none</parallel> |
There was a problem hiding this comment.
Maybe add a comment why parallelism is disabled
| .orTimeout(connectionTimeout, TimeUnit.MILLISECONDS); | ||
|
|
||
| futureClient.thenAccept(client -> { | ||
| String offer = ("\nrelp_version=0\nrelp_software=cfe_35\ncommands=" + "syslog" + "\n"); |
There was a problem hiding this comment.
Do we want to have this as variable in code instead of defined field variable or something? Also why string concatenation as no variables are present
| output.accept(data); | ||
| return 0; | ||
| } | ||
| LOGGER.debug("returning outputReplyFutures.size() <{}>", outputReplyFutures.size()); |
There was a problem hiding this comment.
Debug guard this so .size() won't be repeatedly called
| client.close(); | ||
| } | ||
| catch (Exception e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Will this failure kill the whole process? If not, what happens to the client and other threads?
|
|
||
| RoutingConfig routingConfig = new RoutingConfig(); | ||
| try (TargetRouting targetRouting = new ParallelTargetRouting(routingConfig, this.metricRegistry)) { | ||
| try ( |
There was a problem hiding this comment.
Prefer no try even if it is try-with-resources in tests
|
|
||
| CompletableFuture.allOf(completableFutures).get(); | ||
| } | ||
| catch (ExecutionException | InterruptedException e) { |
There was a problem hiding this comment.
Try/catch in tests, refactor to a better version without them
|
|
||
| CompletableFuture.allOf(completableFutures).get(); | ||
| } | ||
| catch (ExecutionException | InterruptedException e) { |
There was a problem hiding this comment.
Try/catch in tests, refactor to a version without them
|
|
||
| CompletableFuture.allOf(completableFutures).get(); | ||
| } | ||
| catch (ExecutionException | InterruptedException e) { |
There was a problem hiding this comment.
Try/catch in tests, refactor to a version without them
|
|
||
| CompletableFuture.allOf(completableFutures).get(); | ||
| } | ||
| catch (ExecutionException | InterruptedException e) { |
There was a problem hiding this comment.
Try/catch in tests, refactor to a version without them
|
Additional note: I haven't yet tested the compilation or even running the software and the notes above are just from the initial readthrough of the PR, will add more if necessary |
2 participants
No description provided.