[Part 4]: Clean up worker ports #151
Conversation
This diff has two purposes: 1. Establish the semantics of WorkerPorts and make sure all constructors lead to the same behavior (which was not the case before) 2. Make sure WorkerPorts is serializable by both Jackson and Java as it will be sent via wire using java serialization during RPC calls.
sundargates
requested review from
Andyz26,
calvin681,
codyrioux,
hmitnflx,
jeffchao,
nickmahilani and
piygoyal
as code owners
|
|
||
| ext.libraries = [ | ||
| asyncHttpClient: 'org.asynchttpclient:async-http-client:2.12.3', | ||
| commonsLang3: 'org.apache.commons:commons-lang3:3.5', |
There was a problem hiding this comment.
Do you think this one might need to be shaded?
There was a problem hiding this comment.
I don't think so. The breaking API changes in this library are fairly minimum.
| } | ||
|
|
||
| public WorkerPorts(int metricsPort, int debugPort, int consolePort, int customPort, int sinkPort) { | ||
| this.ports = new ArrayList<>(5); |
There was a problem hiding this comment.
could you pl refactor this to call the WorkerPorts(List<Integer>); constructor?
I'd also make ports an immutable because we return the value as-is in getPorts.
| this.consolePort = consolePort; | ||
| this.customPort = customPort; | ||
| this.ports = ports; | ||
| this.sinkPort = ports.get(0); |
| @JsonIgnore | ||
| public List<Integer> getAllPorts() { | ||
| final List<Integer> allPorts = new ArrayList<>(ports); | ||
| final List<Integer> allPorts = new ArrayList<>(); |
There was a problem hiding this comment.
this diff changes the order of the sink port in the list, not sure how this function is used but it might break any code path that relies on sink port being first in list
|
|
||
| @Test public void shouldTokenizeWithEventsContainingPartialDelimiterMatchesWithCustomDelimiter() { | ||
| String delimiter = UUID.randomUUID().toString(); | ||
| String delimiter = "a04f0418-bdff-4f53-af7d-9f5a093b9d65"; |
There was a problem hiding this comment.
I'm curious why the fixed value here?
There was a problem hiding this comment.
This was to get around the library bug fixed in #154. I suppose we can move to randomUUID now.
5 participants
This diff has two purposes:
Context
Explain context and other details for this pull request.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all testsCONTRIBUTING.md