#1314 #1869 Default timeout enhancements

[hogwartsdeveloper]

Closes #1314 #1869

• 1314

• 1869

Proposed Changes

• FileGlobalConfiguration add RequestTimeoutSeconds
• MessageInvokerPool use RequestTimeoutSeconds

[hogwartsdeveloper]

Thank you for PR creation!

Good start! 🚀

But could you work more plz? Please pay attention also that you need to implement requirements of both issues: #1314 and #1869

Thanks, I'll do this for both tasks

[raman-m]

Please find some time to rebase branch onto develop and I'll review. Do not forget to Sync fork to have actual develop before rebasing.

[raman-m]

Additionally, we must consider this https://github.com/ThreeMammals/Ocelot/discussions/2072#discussioncomment-9469641. We have an existing Timeout property as shown here: https://github.com/ThreeMammals/Ocelot/blob/cc8f5c5dfc7c56e4446a2e80d97391429c8b364d/src/Ocelot/Configuration/File/FileRoute.cs#L75 We need to find a way to repurpose it, as it is particularly pertinent to issue #1869

[hogwartsdeveloper]

Additionally, we must consider this #2072 (comment). We have an existing Timeout property as shown here:

https://github.com/ThreeMammals/Ocelot/blob/cc8f5c5dfc7c56e4446a2e80d97391429c8b364d/src/Ocelot/Configuration/File/FileRoute.cs#L75

We need to find a way to repurpose it, as it is particularly pertinent to issue #1869

Ok, I found a solution, can you take a look?

[raman]

Would you stop tagging me?

[hogwartsdeveloper]

@raman-m Write tests?)

[raman-m]

Why did you push the code which cannot be compiled ❓ Build error

Write tests?)

Certainly! But I'm going to review finally...

[raman-m]

@hogwartsdeveloper There is commit: Merge remote-tracking branch 'origin/issues-1869' into issues-1869 Why was it committed? Was it conflict resolution, or something else?

Today I'm going to add another commit to develop, so seems rebasing your feature branch will be necessary, we will see...

[raman-m]

Zhannur, Your PR is the final one pending for the current release/milestone❗

Could you please prepare for an intense session of pair programming this evening and tomorrow whole day? 🙏

I will help you for sure... Please consider an extra DevOps to rebase the feature branch onto develop, if you have time 💡

[raman-m]

Hi @ggnaegi, could you please review the two failed tests in MessageInvokerPoolTests? I'm puzzled as to why they are failing. Here's my attempt to fix them: https://github.com/ThreeMammals/Ocelot/pull/2073/commits/a09760bdf0febae7273184770a0bc28bec8848e1, but unfortunately, it was unsuccessful. 😢 There is something wrong in our current design, or the tests should be developed more.

[raman-m]

@hogwartsdeveloper I'm going to rebase feature branch...

[hogwartsdeveloper]

sorry I didn't have time this evening, I'll find time sorry

[hogwartsdeveloper]

@hogwartsdeveloper There is commit: Merge remote-tracking branch 'origin/issues-1869' into issues-1869 Why was it committed? Was it conflict resolution, or something else?

Today I'm going to add another commit to develop, so seems rebasing your feature branch will be necessary, we will see...

it was a solution to the conflict

[hogwartsdeveloper]

Hi @ggnaegi, could you please review the two failed tests in MessageInvokerPoolTests? I'm puzzled as to why they are failing. Here's my attempt to fix them: a09760b, but unfortunately, it was unsuccessful. 😢 There is something wrong in our current design, or the tests should be developed more.

The tests were failing because there was no timeout, I fixed it, can you take a look?

[ggnaegi]

@raman @hogwartsdeveloper Are you sure about the rebase process, I can see some changes from other PRs, such as Metadata or Version Policy? Otherwise, ok for me, the acceptance tests are still missing though. Acceptance tests for the default timeout, for the priority of the route timeout over global timeout and for the priority of the QoS timeout over route and global timeout should be foreseen imo.

[hogwartsdeveloper]

@raman @hogwartsdeveloper Are you sure about the rebase process, I can see some changes from other PRs, such as Metadata or Version Policy? Otherwise, ok for me, the acceptance tests are still missing though. Acceptance tests for the default timeout, for the priority of the route timeout over global timeout and for the priority of the QoS timeout over route and global timeout should be foreseen imo.

Add tests to PollyQoSResiliencePipelineProvider add test too?

[raman-m]

@hogwartsdeveloper Regarding unit tests... I think we need tests in

• MessageInvokerPoolTests
• PollyQoSResiliencePipelineProviderTests

Regarding acceptance tests...

• PollyQoSTests
• I'm not sure which class is the best to write tests for MessageInvokerPool... Help, @ggnaegi! ContentTests❓

[hogwartsdeveloper]

@hogwartsdeveloper Regarding unit tests... I think we need tests in

• MessageInvokerPoolTests
• PollyQoSResiliencePipelineProviderTests

Regarding acceptance tests...

• PollyQoSTests
• I'm not sure which class is the best to write tests for MessageInvokerPool... Help, @ggnaegi! ContentTests❓

I added tests

• MessageInvokerPoolTests
• RoutesCreatorTests

you can check plz?

[raman-m]

Hi @RaynaldM ! Are you OK if we do the following changes in lines 64-67 of PollyQoSResiliencePipelineProvider:

• branch L64-L67
• PR L64-L67

      var options = route.QosOptions;
      var timeout = options.TimeoutValue;
      if (timeout.HasValue && timeout.Value > 0)
      {
          builder.AddTimeout(TimeSpan.FromMilliseconds(timeout.Value));
      }
      else
      {
          builder.AddTimeout(TimeSpan.FromMilliseconds(_global.QoSOptions.TimeoutValue ?? DefaultQoSTimeoutMilliseconds));
      }

  The change involves a new else block that initializes the TimeOut strategy with a global/default value, ensuring the TimeOut strategy is always enabled. Looks good?

[raman-m]

@ggnaegi If you have time, I would appreciate your review of the failed acceptance tests in the Ocelot.AcceptanceTests.ServiceDiscovery namespace. I've attempted to address them with the newly refactored code (refer to commit 02ab62a43e82be0917452ce2f83f1d2537e34674), but without success. The issues seem to be related to the Consul SD logic, the IInternalConfiguration interface, or QoS. Your fresh perspective might help identify the bug or any design flaws. Thank you!

P.S. I will continue with debugging and a deeper code analysis tomorrow...

[raman-m]

Attention ❗ ⚠️

@hogwartsdeveloper A new commit https://github.com/ThreeMammals/Ocelot/commit/8c1c61e13cd4fe5628e84651eaba9e83f4fed716 in the develop branch has caused numerous merge conflicts in the Polly project files. It's necessary to rebase the feature branch onto develop. I'm starting the rebase process now... your patience is appreciated!

I recommend to create new Backup-branch from your local copy. I will make backup locally too...

P.S.

Could you Sync fork your develop also plz? Because: This branch is 5 commits behind ThreeMammals/Ocelot:develop.

[raman-m]

@hogwartsdeveloper Don't push please❗ But if you want to commit more, then you have to delete branch, fetch all, checkout the branch locally 👉

• git switch develop
• git branch -D issues-1869
• git fetch --all
• git checkout issues-1869

And you will be able to contribute more 😉

[raman-m]

@ggnaegi @RaynaldM

Ready for final code review❗

• @RaynaldM You could pay attention to QoS changes → PollyQoSResiliencePipelineProvider
• @ggnaegi I need your help with broken acceptance tests related to Consul logic (total 3 failed tests) 👇

[raman-m]

Going to rebase onto develop...