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.

Sign up for GitHub

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

Jump to bottom

#1294 Fix the problem while doing first call for service in PollConsul provider #1562

Closed
wants to merge 4 commits into from
Closed

#1294 Fix the problem while doing first call for service in PollConsul provider #1562

wants to merge 4 commits into from

Conversation

przemyslawandruszewski
Copy link

@przemyslawandruszewski przemyslawandruszewski commented Feb 1, 2022 •
edited by raman-m
Loading

Fixes #1294

Fix for PollConsul provider for the first request to any service while API gateway uses PollConsul provider.

Proposed Changes

  • Fix the problem of first request before timer will start fetching services from Consul

@przemyslawandruszewski
Copy link
Author

#1294

@przemyslawandruszewski
Copy link
Author

@TomPallister could you have a look? :)

@przemyslawandruszewski
Copy link
Author

Hi @TomPallister , could you please have a look? :)

@raman-m raman-m added invalid Not actually an issue waiting Waiting for answer to question or feedback from issue raiser labels May 13, 2023
@raman-m
Copy link
Member

raman-m commented May 13, 2023

I could have a look...

Hi Przemyslaw!
Could you resolve merge conflicts first, please?

Don't forget to update your feature branch by latest top-commits from base:develop branch!

@raman-m raman-m marked this pull request as draft May 13, 2023 12:27
@ggnaegi ggnaegi mentioned this pull request Jun 19, 2023
Closed
@raman-m
Copy link
Member

raman-m commented Jun 22, 2023

Przemyslaw!
There are a lot of merge conflicts!
I guess current feature branch is outdated.
It is better to backup your changes to some folder. And re-fork the Ocelot repo. SO, you will have top commits from head repo. And create new feature branch from develop. Apply your code changes. Make new PR please!

Or go to my forked Ocelot repo. And it says: "This branch is up to date with ThreeMammals/Ocelot:develop."
I can add you as Contributor. You will be able to create feature branches.

Do you have an intention to have some pair programming?

@raman-m raman-m force-pushed the bugfix/fix-pollconsul-first-request-problem branch from f602132 to 3888154 Compare September 7, 2023 12:56
@raman-m raman-m marked this pull request as ready for review September 7, 2023 12:57
@raman-m raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance and removed invalid Not actually an issue waiting Waiting for answer to question or feedback from issue raiser labels Sep 7, 2023
@raman-m raman-m changed the title Fix the problem while doing first call for service to gateway using p… Fix the problem while doing first call for service in PollConsul provider Sep 7, 2023
@raman-m raman-m mentioned this pull request Sep 7, 2023
Closed
@raman-m
Copy link
Member

raman-m commented Sep 7, 2023 •
edited
Loading

@przemyslawandruszewski Przemyslaw,
I thought you needed my help to resolve merge conflicts. 😉
The feature branch has been rebased onto ThreeMammals:develop successfully!
I've additionally linked to the issue from your past comment.

Good luck during code review!

@raman-m
Copy link
Member

raman-m commented Sep 7, 2023 •
edited
Loading

@chaitanyaphanikumar @lufeiyuan83 @ggnaegi
Please, join to code review session!

@raman-m raman-m changed the title Fix the problem while doing first call for service in PollConsul provider #1294 Fix the problem while doing first call for service in PollConsul provider Sep 7, 2023
RaynaldM
RaynaldM approved these changes Sep 18, 2023
View reviewed changes
src/Ocelot.Provider.Consul/PollConsul.cs
}

private async Task Poll()
{
_services = await _consulServiceDiscoveryProvider.Get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "Poll" method mixes 2 things: getting services and caching. I think that respecting the principle of single responsibility, and putting caching in the main "Get" method, would make the code more readable.
Going further, is the "Poll" method really useful?

Copy link
Member

Choose a reason for hiding this comment

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

Ray, is caching a good functionality at all in the provider?

@ggnaegi
Copy link
Member

ggnaegi commented Sep 18, 2023

@raman-m This should be addressed in #1670, no?

@raman-m
Copy link
Member

raman-m commented Sep 20, 2023

@ggnaegi commented on Sep 18

Yes, it is addressed, but this PR has extra enhancement of injecting IMemoryCache object into PollConsul constructor! 😉

@ggnaegi
Copy link
Member

ggnaegi commented Sep 21, 2023

@ggnaegi commented on Sep 18

Yes, it is addressed, but this PR has extra enhancement of injecting IMemoryCache object into PollConsul constructor! 😉

Like @RaynaldM I think, we are mixing concerns here...

@raman-m
Copy link
Member

raman-m commented Sep 25, 2023

@ggnaegi By other words, if we remove caching logic here, this PR become duplicate of yours, right?

Yes, it seems caching inside of provider is bad idea. The provider should communicate to Consul service only.
I guess, the author tried to improve performance of the provider, after facing with errors, and this PR is just a try to fix stability of the provider. But this has been addressed in your PR already.

@raman-m
Copy link
Member

raman-m commented Sep 25, 2023

@przemyslawandruszewski
In future we can inject caching interface, but it should not be memory cache, otherwise Ocelot will duplicate all collections from Consul service... I understand, you've tried to improve the performance, but the root cause is
Sorry, cannot accept it nowadays. Cache must be more abstract. And we cannot add this caching forcibly, so configuration property is required.
If you have an intention to contribute, please return back with reopen or message.

Again, Thanks you for the PR!

@raman-m
Copy link
Member

raman-m commented Sep 25, 2023

Duplicate of #1670

@raman-m raman-m marked this as a duplicate of #1670 Sep 25, 2023
@raman-m
Copy link
Member

raman-m commented Sep 25, 2023

#1294 will be fixed by #1670

@raman-m raman-m closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaynaldM RaynaldM RaynaldM approved these changes

@binarymash binarymash Awaiting requested review from binarymash

@jps jps Awaiting requested review from jps

@geffzhang geffzhang Awaiting requested review from geffzhang

@philproctor philproctor Awaiting requested review from philproctor

@anktsrkr anktsrkr Awaiting requested review from anktsrkr

@sharpn sharpn Awaiting requested review from sharpn

@abooraja abooraja Awaiting requested review from abooraja

@wast wast Awaiting requested review from wast

@raman-m raman-m Awaiting requested review from raman-m

@yangzhongke yangzhongke Awaiting requested review from yangzhongke

@thiagoloureiro thiagoloureiro Awaiting requested review from thiagoloureiro

Assignees

@przemyslawandruszewski przemyslawandruszewski

Labels
bug Identified as a potential bug needs feedback Issue is waiting on feedback before acceptance proposal Proposal for a new functionality in Ocelot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ocelot returns 500 from Consul for the first request
4 participants
@przemyslawandruszewski @raman-m @ggnaegi @RaynaldM