Consul name resolution in-memory cache

[a-elsheikh]

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:

• [ ] Code compiles correctly
• [ ] Created/updated tests
• [ ] Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

[yaron2]

@a-elsheikh have you tested this in your environment?

[a-elsheikh]

@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

[pkedy]

@a-elsheikh After the 1.3.0 release, I will spend some cycles to test this on K8s.

[yaron2]

@artursouza can you continue reviewing this please?

[a-elsheikh]

@artursouza any updates on this?

[a-elsheikh]

@artursouza @yaron2 any eyes on this please, I've just attempted to merge master again but these failures I can't resolve

[yaron2]

@a-elsheikh many apologies this has taken so long to review, we'll actively put eyes on this PR now.

Can you please merge with master? /cc @artursouza

[a-elsheikh]

@a-elsheikh many apologies this has taken so long to review, we'll actively put eyes on this PR now.

Can you please merge with master? /cc @artursouza

no worries, thank you @yaron2

[dapr-bot]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[a-elsheikh]

hey @yaron2 @artursouza - any updates on this please. We're keen on utilizing this feature as part of our adoption strategy

[dapr-bot]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[dapr-bot]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[a-elsheikh]

@artursouza can you have a look please

[artursouza]

Reopening

[chenjpu]

[a-elsheikh]

@yaron2 @artursouza any updates please :)

[badgeratu]

@artursouza @yaron2 I believe we addressed your review comments on this PR some time ago, with 1.8.0 now out the door could you please pick this up and prioritize it again as we do need this for our next phase of Dapr rollout.

[berndverst]

@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[bot]]

Codecov Report

Patch coverage: 90.47% and project coverage change: +2.09% :tada:

Comparison is base (33a5fd2) 34.97% compared to head (c7d9e20) 37.07%.

:exclamation: Current head c7d9e20 differs from pull request most recent head 803d29d. Consider uploading reports for the commit 803d29d to get more accurate results

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     

Files Changed	Coverage Δ
nameresolution/consul/consul.go	79.01% <87.80%> (+5.33%)	:arrow_up:
nameresolution/consul/watcher.go	92.07% <92.07%> (ø)
nameresolution/consul/configuration.go	96.13% <100.00%> (+1.44%)	:arrow_up:

... and 228 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ItalyPaleAle]

It looks like this PR is outdated and probably irrelevant at this point. Ok to close this? CC: @berndverst

[badgeratu]

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.

[ItalyPaleAle]

Ok, thanks for confirming. If you can fix the merge conflicts, I'll review it for Dapr 1.11.

[alberthuang24]

I also really need this feature

[berndverst]

@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. It's very difficult to merge PRs where we as maintainers do not have automatic write access to the remote fork.

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.

[a-elsheikh]

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.

[berndverst]

@a-elsheikh @badgeratu can you please ping me in Discord (@bernd) once you have resolved merge conflicts and fixed DCO here?

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!

[ItalyPaleAle]

Closing as it's been re-created as #3121