-
Notifications
You must be signed in to change notification settings - Fork 463
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
Consul name resolution in-memory cache #963
Conversation
@a-elsheikh have you tested this in your environment? |
In self-hosted mode, yes - it would be best if someone could evaluate the behaviour on k8s |
@a-elsheikh After the 1.3.0 release, I will spend some cycles to test this on K8s. |
@artursouza can you continue reviewing this please? |
@artursouza any updates on this? |
@a-elsheikh could you please merge master into the branch again to address the potential conflict? I tried pushing a commit but it does not appear you are giving maintainers edit access to your PR / branch. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #963 +/- ##
==========================================
+ Coverage 34.97% 37.07% +2.09%
==========================================
Files 241 212 -29
Lines 29887 26904 -2983
==========================================
- Hits 10454 9975 -479
+ Misses 18552 16152 -2400
+ Partials 881 777 -104
☔ View full report in Codecov by Sentry. |
It looks like this PR is outdated and probably irrelevant at this point. Ok to close this? |
It's definitely not outdated, we've been waiting patiently for a re-review as we believe we've addressed all comments. If you could push that it would be greatly appreciated, this is still a vital feature required by us. |
Ok, thanks for confirming. If you can fix the merge conflicts, I'll review it for Dapr 1.11. |
I also really need this feature |
@badgeratu because we do not have write access to your fork we cannot shepherd this PR -- so this PR will constantly encounter merge conflicts that only you can resolve. Can you invite contrib maintainers directly to have write access to your fork so we can unblock this pr (it seems you have disabled the feature to give repo maintainers this ability automatically at an organizational level)? The maintainers are: @ItalyPaleAle @yaron2 @berndverst @artursouza We are days (or maybe just one day) away from code freeze for 1.11. If we find that tests pass after addressing merge conflicts and updating the branch then this is likely good to go - but we'll re-review then. |
hey @berndverst thanks for reaching out - as this fork exists under the corporate area we've limited flexibility in amending the permissions. In any case, I've got eyes on this now so am happy to iterate with the maintainers to see it done now that there is engagement. |
@a-elsheikh @badgeratu can you please ping me in Discord ( I will try to get this merged - we are now at code freeze for 1.12 - but if you can get this resolved Monday we can still try to get it in! |
Signed-off-by: Abdulaziz Elsheikh <abdulaziz.elsheikh@gmail.com>
Closing as it's been re-created as #3121 |
Description
Added service cache to consul nr component
Issue reference
#934
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: