fix condition for KubePortManagerSingleton init twice #16213
Conversation
|
|
github-actions
bot
added
area/platform
|
Can you sign the CLA @goodseeyou ? |
|
@davinchia can you check this contribution? |
|
Hi @marcosmarxm I signed it |
|
@goodseeyou can you explain the error case this logic change is catching? I want to make sure I understand what you are thinking. We have pretty good test today in |
| @@ -121,6 +119,31 @@ public void teardown() throws Exception { | |||
| server.stop(); | |||
| } | |||
|
|
|||
| @RetryingTest(3) | |||
| public void testInitKubePortManagerSingletonTwice() throws Exception { | |||
There was a problem hiding this comment.
@davinchia what I really add test here.
Others code change is coding-style formatter by google-style which recommends (https://github.com/airbytehq/airbyte/blob/master/docs/contributing-to-airbyte/code-style.md)
goodseeyou
force-pushed
the
fixInitTwiceBug
branch
from
94d81da
to
f24abf1
Compare
| @@ -56,7 +56,7 @@ public static synchronized KubePortManagerSingleton getInstance() { | |||
| public static synchronized void init(final Set<Integer> ports) { | |||
| if (instance == null) { | |||
| instance = new KubePortManagerSingleton(ports); | |||
| } else if (Sets.intersection(instance.getAllPorts(), ports).size() != ports.size()) { | |||
There was a problem hiding this comment.
here is what I really fix
goodseeyou
force-pushed
the
fixInitTwiceBug
branch
from
c42f192
to
48eae83
Compare
|
local test successfully |
|
@davinchia can you check this PR? |
|
@goodseeyou thank you! |
...kers/src/test-integration/java/io/airbyte/workers/process/KubePodProcessIntegrationTest.java
Outdated
Show resolved
Hide resolved
...kers/src/test-integration/java/io/airbyte/workers/process/KubePodProcessIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@goodseeyou two small spelling suggestions. After that, good to merge!
|
Hi @davinchia, I fixed the spelling by your suggestions. |
|
Sorry the delay here @goodseeyou I'll merge this on Monday. |
There was a problem hiding this comment.
It is building locally! Thanks @goodseeyou sorry the delay here.
* add test for init ports manager twice * fix bug for init more then onece * formatter * fix typo by pr suggestions Co-authored-by: Davin Chia <davinchia@gmail.com>
What
Fix condition to init KubePortManagerSingleton twice when given ports are the same.
How
make the condition align to the log message
Recommended reading order
airbyte-workers/src/main/java/io/airbyte/workers/process/KubePortManagerSingleton.javaPre-merge Checklist
Expand the relevant checklist and delete the others.
Tests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.