fix: configure idleTimeoutUnit as MILLISECONDS to avoid long client wait

[lamluongbinh]

Fixes #4342

Details:

• configure idleTimeoutUnit as MILLISECONDS to avoid long client wait
• Docs: link
• TimeUnit: link

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: lamluongbinh / name: Michael. Lam (8fd945b, 3f4b503, 116bdec, 270c24a)

[knative-prow]

Hi @lamluongbinh. Thanks for your PR.

I'm waiting for a knative-extensions member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[creydr]

Thanks a lot @lamluongbinh for this fix.
I only had one comment so far to make the linter happy.

Could you also include
Fixes #4342

In your PR description, so the issue gets linked and closed, when this PR gets merged.

/ok-to-test

[lamluongbinh]

Thanks @creydr for your review. I've already made the fixes you mentioned

[creydr]

Thanks @creydr for your review. I've already made the fixes you mentioned

Thanks for updating it @lamluongbinh . Can you also check on your CLA (#4367 (comment))?

[matzew]

/test upgrade-tests

[matzew]

Shouldn't we make it consistent, everywhere?

E.g. here is also that idle settings:
https://github.com/lamluongbinh/eventing-kafka-broker/blob/chore/set-idle-timeout-unit-milliseconds/data-plane/config/broker/100-config-kafka-broker-data-plane.yaml#L83

[matzew]

/lgtm
/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lamluongbinh, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [matzew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.99%. Comparing base (4f920dd) to head (0f7d42e).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4367      +/-   ##
==========================================
- Coverage   30.01%   29.99%   -0.03%     
==========================================
  Files         290      290              
  Lines       19328    19351      +23     
==========================================
+ Hits         5801     5804       +3     
- Misses      13073    13094      +21     
+ Partials      454      453       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[matzew]

@lamluongbinh see the verify task:

Error: Please run ./hack/update-codegen.sh.

[knative-prow]

New changes are detected. LGTM label has been removed.

[lamluongbinh]

Hey @matzew ,

I've run the ./hack/update-codegen.sh script and committed the generated files to the MR. Can you confirm this matches your expectations, or is there anything else you'd like me to adjust?

Thanks!

[creydr]

/retest-required

[creydr]

@lamluongbinh your PR still needs the CLA authorization (see #4367 (comment))

[matzew]

/retest-required

[matzew]

/retest-required

[matzew]

/test upgrade-tests

[matzew]

/test reconciler-tests

[matzew]

/hold

@lamluongbinh these vendor files should not be there.

[matzew]

Looks like it is now outdated and needs a rebase.

I think that is due to some incorrect merges ....

IMO best approach is:

• delete your local branch
• create a new one based out of main (but use the same same name)
• git cherrypick these:
  • 5793e05
  • 0f7d42e
• run ./hack/update-codegen.sh
• git push force the new branch to your remote one

[lamluongbinh]

@matzew the vendor files are generated by this

I've just rebase it. Please take a look

[lamluongbinh]

Hi @matzew,

I've just found this: link.

There is a delivery.timeout feature currently in beta, but it isn’t widely documented. DEFAULT_TIMEOUT_MS is set to 600,000L, meaning each request defaults to a 600-second timeout if the trigger doesn't explicitly specify one.

If we set the idleTimeout too short, it might close the connection prematurely—before receiving any response from the client—and override the default timeout if the trigger doesn't specify one.

Do you think we could increase the default idleTimeout to a longer duration, such as 30 minutes, instead of the change I maded - 10,000 ms?

We will also need clearer documentation about the default timeout in our current setup. Additionally, regarding delivery timeout - link. It might be helpful to reference this and include it in our documentation as well.

[lamluongbinh]

Hi @matzew,

I've just finalized the setup on my end and tested the configuration in my system.

Currently, we've adjusted the idle timeout setting to 30 minutes. By default, Istio sets the idle timeout to 1 hour, but I wanted a more stringent limit while ensuring it doesn't interfere with the default 10-minute timeout for sending messages. The 30-minute setting provides a balance between preserving connection usability and reducing idle time.

I’ve also kept the comment in the configuration to clarify that the timeout value is in seconds by default, to prevent any potential confusion for others reviewing or updating it later.

Let me know if you have any concerns or further suggestions.

[lamluongbinh]

/retest-required

[Cali0707]

/retest-required