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

Update core and has dhstore as a valuestore #1989

Merged
merged 2 commits into from
Jun 16, 2023

Conversation

gammazero
Copy link
Collaborator

Context

The new core has dhstore as a valuestore implementation, and not as an option to the engine.

Proposed Changes

  • Use latest core with dhstore valuestore instance.
  • Make value store required.
  • If valuestore is "" or "none", and DHStoreURL is configured, then use dhstore as valuestore. This makes upgrade compatible with previous configs.

Tests

  • Update e2e test to have 1st indexer write to pebble and 2nd write to dhstore.

Revert Strategy

Change is safe to revert.

The new core has dhstore as a valuestore implementation, and not as an option to the engine.

- Make value store required.
- If valuestore is "" or "none", and DHStoreURL is configured, then use dhstore as valuestore. This makes upgrade compatible with previous configs.
- Update e2e test to have 1st indexer write to pebble and 2nd write to dhstore.
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

Patch coverage: 2.94% and project coverage change: -1.44 ⚠️

Comparison is base (578cd50) 47.75% compared to head (2c6efe3) 46.31%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1989      +/-   ##
==========================================
- Coverage   47.75%   46.31%   -1.44%     
==========================================
  Files          95       95              
  Lines       10250    10251       +1     
==========================================
- Hits         4895     4748     -147     
- Misses       4782     4947     +165     
+ Partials      573      556      -17     
Impacted Files Coverage Δ
command/daemon.go 0.00% <0.00%> (ø)
config/indexer.go 47.50% <0.00%> (-3.86%) ⬇️
internal/registry/registry.go 63.18% <0.00%> (-0.20%) ⬇️
command/init.go 48.03% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gammazero gammazero requested a review from ischasny June 16, 2023 09:12
@ischasny
Copy link
Collaborator

Hi Andrew, thanks for the PR. What would be the migration plan for existing indexers? I.e. how would they move their value stores over to pebble?

@gammazero
Copy link
Collaborator Author

gammazero commented Jun 16, 2023

@ischasny There is no migration plan.

This PR removes the hybrid configuration where an indexer writes to both pebble and dhstore, since there should no longer be any need to support this.

For indexers that are writing to dhstore:

This is completely compatible with their current configurations. All indexers that are writing to dhstore have no local pebble valuestore, and we would not want to migrate them to pebble.

For all other indexers that are writing to pebble:

There is no need to change anything. They will continue writing to pebble and will eventually be replaced by indexers that write to dhstore.

@gammazero gammazero merged commit 272f579 into main Jun 16, 2023
16 checks passed
@gammazero gammazero deleted the update-core-dhstore-as-valstore branch June 16, 2023 21:32
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.

None yet

3 participants