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

test_out_forward: remove unnecessary ack_response_timeout setting #4685

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

Watson1978
Copy link
Contributor

@Watson1978 Watson1978 commented Oct 28, 2024

Which issue(s) this PR fixes:
Fixes #4684

What this PR does / why we need it:
Seems that timeout setting is short in ack_response_timeout.
Seems that It may take some time to receive a ACK response
so the process in ack handler has expired and the node is disabled.

This PR will remove unnecessary ack_response_timeout settings for the test

Docs Changes:

Release Note:

Seems that timeout setting is short in ack_response_timeout.
Seems that It may take some time to receive a ACK response
so the process in ack handler has expired and the node is disabled.

Signed-off-by: Watson <[email protected]>
@Watson1978 Watson1978 marked this pull request as ready for review October 28, 2024 00:51
@daipom daipom self-requested a review October 28, 2024 06:52
@daipom
Copy link
Contributor

daipom commented Oct 28, 2024

There are still some unclear points for me:

#4684 (comment)

@daipom
Copy link
Contributor

daipom commented Oct 29, 2024

Thanks for this enhancement!
Now, the issue is clear to me (#4684).

Indeed, the timeout is short.

How about removing the timeout setting?

Both this setting and the following setting would be unnecessary for the tests.

a node supporting responses after stop

ack_response_timeout 10s

How about removing these 2 setting?

@Watson1978 Watson1978 changed the title test_out_forward: relax an ack_response_timeout test_out_forward: remove unnecessary ack_response_timeout setting Oct 29, 2024
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@daipom daipom merged commit 17005d5 into fluent:master Oct 29, 2024
14 of 16 checks passed
@Watson1978 Watson1978 deleted the fix-no_nodes_available branch October 29, 2024 08:03
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.

CI: Fails with "Fluent::Plugin::ForwardOutput::NoNodesAvailable: no nodes are available"
2 participants