Skip to content

Support MSB storage #8954

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support MSB storage #8954

wants to merge 1 commit into from

Conversation

guy-har
Copy link
Contributor

@guy-har guy-har commented Apr 9, 2025

Closes treeverse/lakeFS-enterprise#48

Change Description

This change provides the lakeFSFS to support storage config list case
This change:

  • uses the config endpoint instead of the depricated internal getStorageConfig endpoint
  • Uses getStorageConfigList in case it's provided, otherwise fallbacks to storageConfig

How was this tested

This was tested by running the sonnets test locally with the following information:

  1. Use lakeFSFS instead of gateway
  2. Use on lakeFS running with MSB without a bc storage

@guy-har guy-har requested review from N-o-Z and arielshaqed April 9, 2025 10:01
Copy link

github-actions bot commented Apr 9, 2025

E2E Test Results - Quickstart

12 passed

Copy link

github-actions bot commented Apr 9, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed, 1 skipped

@guy-har guy-har force-pushed the task/lakefsfs-msb-support branch from 95f93ab to a36cbe9 Compare April 9, 2025 15:01
@guy-har guy-har added the include-changelog PR description should be included in next release changelog label Apr 10, 2025
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

LGTM!

storageConfig = storageConfigList.stream()
.filter(sc -> Objects.equals(sc.getBlockstoreId(), storageID))
.findFirst()
.orElseThrow(() -> new IOException("Failed to get lakeFS blockstore type"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.orElseThrow(() -> new IOException("Failed to get lakeFS blockstore type"));
.orElseThrow(() -> new IOException("Failed to get lakeFS blockstore configuration"));

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Cool, thanks! Only minor changes requested.

@@ -287,7 +287,7 @@ To export to S3:
<dependency>
<groupId>io.lakefs</groupId>
<artifactId>sdk</artifactId>
<version>1.18.0</version>
<version>1.53.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking. As always, I prefer big version bumps to occur in separate PRs. Otherwise if things break, it is harder to tell if the breakage was due to the version bump or due to the change in functionality.

.execute();
String storageID = repository.getStorageId();
Config cfg = lfsClient.getConfigApi().getConfig().execute();
StorageConfig storageConfig = new StorageConfig();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand - why create a blank StorageConfig? If there is a bug and nothing is found, do you really want a blank StorageConfig???

Suggested change
StorageConfig storageConfig = new StorageConfig();
StorageConfig storageConfig;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants