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

Controller populate storage ID #8714

Merged
merged 6 commits into from
Feb 25, 2025
Merged

Controller populate storage ID #8714

merged 6 commits into from
Feb 25, 2025

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Feb 25, 2025 •
edited
Loading

Controller, populate storage ID with actual value when returning repository information

@N-o-Z N-o-Z added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached msb labels Feb 25, 2025
@N-o-Z N-o-Z self-assigned this Feb 25, 2025
itaigilo
itaigilo reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

Very nice.

Approving,
But I think that making the StorageConfig part of the ref.ManagerConfig struct will make things cleaner.

pkg/catalog/catalog.go Outdated
@@ -363,7 +364,8 @@ func New(ctx context.Context, cfg Config) (*Catalog, error) {
CommitCacheConfig: ref.CacheConfig(baseCfg.Graveler.CommitCache),
MaxBatchDelay: baseCfg.Graveler.MaxBatchDelay,
BranchApproximateOwnershipParams: makeBranchApproximateOwnershipParams(baseCfg.Graveler.BranchOwnership),
})
}, cfg.Config.StorageConfig(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: new line,
So it's easier to understand to which func this is passed.

pkg/graveler/ref/manager.go Outdated
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"github.com/treeverse/lakefs/pkg/config"
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports order.

itaigilo
itaigilo reviewed Feb 25, 2025
View reviewed changes
pkg/graveler/ref/manager.go Outdated
@@ -142,6 +145,10 @@ func (m *Manager) getRepository(ctx context.Context, repositoryID graveler.Repos
}
return nil, err
}
repo := graveler.RepoFromProto(&data)
if repo.StorageID == config.SingleBlockstoreID {
repo.StorageID = graveler.StorageID(m.storageConfig.GetStorageByID(config.SingleBlockstoreID).ID()) // Will return the real actual ID
Copy link
Contributor

@itaigilo itaigilo Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Noting that on server start storageConfig.GetStorageByID() is checked for each repo, so it can't return null here (which is obviously good).

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, no -
The checkRepos in run.go uses ListRepositories() for this validation.
So in case of a bad config, we'll get a nil panic here, instead of a nice message.

Any idea how to handle this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

itaigilo
itaigilo requested changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@itaigilo itaigilo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Requesting changes because of an error handling issue we have to solve...

pkg/graveler/ref/manager.go Outdated
@@ -142,6 +145,10 @@ func (m *Manager) getRepository(ctx context.Context, repositoryID graveler.Repos
}
return nil, err
}
repo := graveler.RepoFromProto(&data)
if repo.StorageID == config.SingleBlockstoreID {
repo.StorageID = graveler.StorageID(m.storageConfig.GetStorageByID(config.SingleBlockstoreID).ID()) // Will return the real actual ID
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, no -
The checkRepos in run.go uses ListRepositories() for this validation.
So in case of a bad config, we'll get a nil panic here, instead of a nice message.

Any idea how to handle this?

@github-actions GitHub Actions
Copy link

E2E Test Results - Quickstart

11 passed

@github-actions GitHub Actions
Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

14 passed

@N-o-Z N-o-Z requested a review from itaigilo February 25, 2025 17:16
@N-o-Z N-o-Z changed the title Graveler populate storage ID Controller populate storage ID Feb 25, 2025
@N-o-Z N-o-Z force-pushed the task/populate-storage-id-in-graveler branch from 24867f1 to b357485 Compare February 25, 2025 18:15
itaigilo
itaigilo approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

Looking good.
I like it better in the controller, seems like a more natural choice than in Graveler.

Approving,
With two questions:

  1. Any reason GetRepository doesn't use getActualStorageID?
  2. Can we cover this change (even in a minimal way) with tests?

@N-o-Z
Copy link
Member Author

N-o-Z commented Feb 25, 2025

Looking good. I like it better in the controller, seems like a more natural choice than in Graveler.

Approving, With two questions:

  1. Any reason GetRepository doesn't use getActualStorageID?
  2. Can we cover this change (even in a minimal way) with tests?
  1. It does use it
  2. It's covered in the tests - we will need to test this extensively elsewhere

@N-o-Z N-o-Z merged commit 4542f08 into master Feb 25, 2025
41 checks passed
@N-o-Z N-o-Z deleted the task/populate-storage-id-in-graveler branch February 25, 2025 19:04
@itaigilo itaigilo mentioned this pull request Feb 25, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itaigilo itaigilo itaigilo approved these changes

@nopcoder nopcoder Awaiting requested review from nopcoder

@guy-har guy-har Awaiting requested review from guy-har

Assignees

@N-o-Z N-o-Z

Labels
exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached msb
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@N-o-Z @itaigilo