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

#1314 #1869 #2072 Default timeout enhancements #2073

Open
wants to merge 50 commits into
base: develop
Choose a base branch
from

Conversation

hogwartsdeveloper
Copy link
Contributor

@hogwartsdeveloper hogwartsdeveloper commented May 25, 2024

@raman-m raman-m changed the title RequestTimeoutSeconds FileGlobalConfiguration #1869 RequestTimeoutSeconds FileGlobalConfiguration May 26, 2024
@raman-m raman-m linked an issue May 26, 2024 that may be closed by this pull request
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

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

src/Ocelot/Configuration/File/FileGlobalConfiguration.cs Outdated Show resolved Hide resolved
src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved
src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved
src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs Outdated Show resolved Hide resolved
@raman-m raman-m changed the title #1869 RequestTimeoutSeconds FileGlobalConfiguration #1314 #1869 Default timeout enhancements May 26, 2024
@hogwartsdeveloper
Copy link
Contributor Author

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

@hogwartsdeveloper hogwartsdeveloper requested a review from raman-m May 27, 2024 11:37
@raman-m
Copy link
Member

raman-m commented May 28, 2024

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 raman-m requested review from RaynaldM and ggnaegi May 28, 2024 09:34
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

2nd round has finished! My suggestions are below.

src/Ocelot.Provider.Polly/v7/PollyQoSProvider.cs Outdated Show resolved Hide resolved
src/Ocelot/DependencyInjection/OcelotBuilder.cs Outdated Show resolved Hide resolved
src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved
@raman-m
Copy link
Member

raman-m commented May 28, 2024

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

public int Timeout { get; set; }

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

@hogwartsdeveloper
Copy link
Contributor Author

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

public int Timeout { get; set; }

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?

@hogwartsdeveloper hogwartsdeveloper requested a review from raman-m May 29, 2024 05:49
Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

Hello, thanks a lot, we should clarify the logic though. In my opinion, we should set a default global timeout value in the FileGlobalConfiguration and handle everything in the ITimeoutCreator implementation.

src/Ocelot/Configuration/File/FileGlobalConfiguration.cs Outdated Show resolved Hide resolved
src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved
src/Ocelot/Requester/MessageInvokerPool.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/TimeoutCreator.cs Outdated Show resolved Hide resolved
@hogwartsdeveloper hogwartsdeveloper requested a review from ggnaegi May 29, 2024 09:20
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

I will continue to make suggestions without the expectation of immediate implementation.
We are currently facing design conflicts... 😢

@hogwartsdeveloper, please exercise patience! Our discussions may be lengthy, and delivering both linked features will be challenging. I am optimistic that we will reach a consensus on the current design and necessary refactoring.

At present, it is imperative to include this PR in the current release. The community has been eagerly awaiting the "default timeout feature", which is a high-priority item. Therefore, I will assign this PR to the Spring'24 milestone.

src/Ocelot.Provider.Polly/v7/PollyQoSProvider.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Builder/DownstreamRouteBuilder.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/TimeoutCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/DownstreamRoute.cs Show resolved Hide resolved
src/Ocelot/DependencyInjection/OcelotBuilder.cs Outdated Show resolved Hide resolved
@raman-m raman-m added feature A new feature QoS Ocelot feature: Quality of Service aka Polly Configuration Ocelot feature: Configuration Routing Ocelot feature: Routing Spring'24 Spring 2024 release labels May 29, 2024
@raman-m raman-m added this to the Spring'24 milestone May 29, 2024
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

@hogwartsdeveloper
Here's a summary of today's team discussions:

  • Change all int return types to int? for the properties: FileRoute.Timeout, FileGlobalConfiguration.TimeoutSeconds, and FileQoSOptions.TimeoutValue. We've reached a consensus to make them nullable, which will enable us to write cleaner code without gaps in business logic.
  • Relocate the ITimeoutCreator.Create method to the IRoutesCreator interface as the CreateTimeout method. The ITimeoutCreator micro-interface seems unnecessary with only one reference and one injection. Moreover, the implementation is one line of code. It will be tested as part of the RoutesCreator class.

These are the essential changes required❗

src/Ocelot/Configuration/Creator/ITimeoutCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/RoutesCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/RoutesCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/TimeoutCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/File/FileGlobalConfiguration.cs Outdated Show resolved Hide resolved
src/Ocelot/DependencyInjection/OcelotBuilder.cs Outdated Show resolved Hide resolved
Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

@raman-m Thanks a a lot @hogwartsdeveloper, I think the QoS issues are not part of this current PR scope, we should address it in a later PR. We are almost there, but it would be wise to use the same property names for the Timeout on the global configuration and the route configuration, with milliseconds precision. Some Acceptance tests are missing (Testing if the Global configuration Timeout is working as expected, that the QoS Timeout value has priority over the global / route Timeout value).

src/Ocelot/Configuration/File/FileGlobalConfiguration.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/RoutesCreator.cs Outdated Show resolved Hide resolved
hogwartsdeveloper and others added 27 commits November 1, 2024 18:08
…Route`, which determines this value based on the QoS mode.

The `DefaultTimeoutSeconds` should be defined in a single reusable location, currently within `DownstreamRoute`.
Add the NoWait test helper, which is based on the Retry pattern.
…se it is singleton service!

Make default timeout properties static.
@raman-m
Copy link
Member

raman-m commented Nov 1, 2024

TODO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Ocelot feature: Configuration Dec'24 December 2024 release feature A new feature highest Highest priority QoS Ocelot feature: Quality of Service aka Polly Routing Ocelot feature: Routing
Projects
None yet
5 participants