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

Local file-based name resolver with SQLite + Update components-contrib to register Blob Storage v2 #7038

Merged
merged 12 commits into from Nov 6, 2023

Conversation

ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle commented Oct 13, 2023

Fixes #3256

This PR updates components-contrib to the latest commit, so it also requires registering the new Azure Blob Storage v2 components

Addresses a longstanding request (> 2 years!) to have an alternative name resolver component for local development, for situations where mDNS can't be used (and even in those situations, a local name resolver is more convenient for local development).

As discussed in dapr/components-contrib#1380, this is implemented with SQLite, which is fast and efficient.

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle requested review from a team as code owners October 13, 2023 23:05
go.mod Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (90835fa) 65.01% compared to head (44033e5) 65.07%.

❗ Current head 44033e5 differs from pull request most recent head 437cde1. Consider uploading reports for the commit 437cde1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7038      +/-   ##
==========================================
+ Coverage   65.01%   65.07%   +0.05%     
==========================================
  Files         221      231      +10     
  Lines       20799    21060     +261     
==========================================
+ Hits        13523    13704     +181     
- Misses       6138     6214      +76     
- Partials     1138     1142       +4     
Files Coverage Δ
pkg/messaging/direct_messaging.go 55.63% <0.00%> (ø)
pkg/runtime/runtime.go 73.18% <86.36%> (+0.03%) ⬆️

... and 23 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@JoshVanL JoshVanL left a comment

Choose a reason for hiding this comment

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

One thing, otherwise LGTM 👍

Could you add a simple service invocation integration test using SQLite?

pkg/messaging/direct_messaging.go Show resolved Hide resolved
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle marked this pull request as draft October 17, 2023 15:40
@ItalyPaleAle
Copy link
Contributor Author

The PR in contrib was merged so this is good to go!! 🚀

Could you add a simple service invocation integration test using SQLite?

No, we should not use tests in the runtime to validate a component. There are some “ITs” for this component in components-contrib (implemented as unit tests because we don’t have an infra to do otherwise). We should add an infra in components-contrib to test nameresolvers, also for the other nameresolvers that don’t have a test (Consul)

@ItalyPaleAle ItalyPaleAle marked this pull request as ready for review November 1, 2023 23:21
nr.AppPort: strconv.Itoa(a.runtimeConfig.appConnectionConfig.Port),
nr.HostAddress: a.hostAddress,
nr.AppID: a.runtimeConfig.id,
resolverMetadata.Instance = nr.Instance{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A small API change in contrib. Now properties such as address and port are passed in a strongly-typed struct rather than in a weakly-typed map.

yaron2
yaron2 previously requested changes Nov 2, 2023
go.mod Outdated Show resolved Hide resolved
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@yaron2
Copy link
Member

yaron2 commented Nov 2, 2023

Integration tests seem to be consistently failing. I reran the tests 3 times. Are there known issues in master branch with integration tests? cc @JoshVanL

@ItalyPaleAle
Copy link
Contributor Author

Integration tests seem to be consistently failing. I reran the tests 3 times. Are there known issues in master branch with integration tests? cc @JoshVanL

Looks like the test that's failing is the reminders rebalancing, and it's failing consistently in this PR and also when I run it locally. It appears the issue is that placement table dissemination doesn't happen in time.

It's really weird as this PR doesn't touch anything related to placement 😮

I am investigating

@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented Nov 2, 2023

The issue seems to be an unrelated bug introduced by a separate commit in components-contrib. I'm going to fix or revert that commit.

--EDIT Fixed

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle changed the title Local file-based name resolver with SQLite Local file-based name resolver with SQLite + Update components-contrib to register Blob Storage v2 Nov 3, 2023
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@yaron2 yaron2 merged commit c2cbcd0 into dapr:master Nov 6, 2023
16 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Provide alternative service resolution mechanism besides mdns
3 participants