Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: advertised address should fall back to configured address #9669

Merged
merged 3 commits into from
Jul 3, 2022

Conversation

lenaschoenburg
Copy link
Member

Description

Since we introduced support for advertised addresses in the gateway in #9572, the atomix communication services were bound to the default host and port (0.0.0.0:26502) if no advertised address was configured instead of falling back to the configured host and port first.

Related issues

closes #9658 because job available notifications were not received which caused the delay in job activation.

If no advertised host and port are configured, we need to fall back to
the configured host and port and not to the default host and port.
Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ole 👍

I would like to have an integration test which fails without the fix actually, and/or that we use the StandaloneVateway setup code in our QA IT ClusterRule. Not sure whether we want to do that as follow up but it would be good to have it.


// then
assertThat(actual.getCluster().getAdvertisedHost()).isEqualTo(expectedHost);
assertThat(actual.getCluster().getAdvertisedPort()).isEqualTo(Integer.parseInt(expectedPort));

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException

Potential uncaught 'java.lang.NumberFormatException'.
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2022

Unit Test Results

   785 files  +  1     785 suites  +1   1h 35m 14s ⏱️ + 2m 57s
5 579 tests +59  5 572 ✔️ +59  7 💤 ±0  0 ±0 
5 751 runs  +59  5 744 ✔️ +59  7 💤 ±0  0 ±0 

Results for commit 46ece38. ± Comparison against base commit b51f397.

♻️ This comment has been updated with latest results.

@lenaschoenburg
Copy link
Member Author

I agree, but I'd like to do this as a follow up since I'd like to include this fix in the next alpha release. See #9670

Add regression test which fails on SNAPSHOT, the job notification is missed in that case. With the fix the test is green.
@Zelldon
Copy link
Member

Zelldon commented Jul 1, 2022

I created an integration test (regression test), which fails with SNAPSHOT and is green with gcr.io/zeebe-io/zeebe:8.1.0-SNAPSHOT-os-debug-longpolling-notif-b51f3971. Please have a look whether this is fine with you. I hope it was ok for you that I did that.

Co-authored-by: Nicolas Pepin-Perreault <43373+npepinpe@users.noreply.github.com>
@lenaschoenburg
Copy link
Member Author

bors r+

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit f2f5341 into main Jul 3, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the fix-gateway-advertised-address branch July 3, 2022 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Job activation delayed by 10 seconds under stable network conditions
3 participants