[go: nahoru, domu]

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 Default timeout enhancements #2073

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

Conversation

hogwartsdeveloper
Copy link
Contributor
@hogwartsdeveloper hogwartsdeveloper commented May 25, 2024

Closes #1314 #1869

Proposed Changes

  • FileGlobalConfiguration add RequestTimeoutSeconds
  • MessageInvokerPool use RequestTimeoutSeconds

@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

@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?

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
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.

@raman-m raman-m added feature A new feature QoS Ocelot feature: Quality of Service 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
@raman
Copy link
raman commented May 29, 2024

Would you stop tagging me?

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2023 Annual 2023 release Configuration Ocelot feature: Configuration feature A new feature QoS Ocelot feature: Quality of Service Routing Ocelot feature: Routing
Projects
None yet
5 participants