-
Notifications
You must be signed in to change notification settings - Fork 37
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
Refactor: Remove FsStorageProvider from Table Service #104
Conversation
...s-test-fixtures/src/main/java/com/linkedin/openhouse/tablestest/SpringH2TestApplication.java
Show resolved
Hide resolved
cc: @ctrezzo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good, few nits/comments
cluster/storage/src/main/java/com/linkedin/openhouse/cluster/storage/StorageClient.java
Outdated
Show resolved
Hide resolved
...r/storage/src/main/java/com/linkedin/openhouse/cluster/storage/local/LocalStorageClient.java
Show resolved
Hide resolved
...les/src/main/java/com/linkedin/openhouse/tables/repository/impl/InternalRepositoryUtils.java
Outdated
Show resolved
Hide resolved
services/tables/src/main/java/com/linkedin/openhouse/tables/utils/TableUUIDGenerator.java
Outdated
Show resolved
Hide resolved
...s-test-fixtures/src/main/java/com/linkedin/openhouse/tablestest/SpringH2TestApplication.java
Show resolved
Hide resolved
cluster/storage/src/main/java/com/linkedin/openhouse/cluster/storage/StorageClient.java
Outdated
Show resolved
Hide resolved
...ernalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/fileio/FileIOManager.java
Show resolved
Hide resolved
services/tables/src/main/java/com/linkedin/openhouse/tables/config/MainApplicationConfig.java
Show resolved
Hide resolved
...les/src/main/java/com/linkedin/openhouse/tables/repository/impl/InternalRepositoryUtils.java
Outdated
Show resolved
Hide resolved
services/tables/src/main/java/com/linkedin/openhouse/tables/utils/TableUUIDGenerator.java
Show resolved
Hide resolved
...r/storage/src/main/java/com/linkedin/openhouse/cluster/storage/local/LocalStorageClient.java
Show resolved
Hide resolved
services/tables/src/main/java/com/linkedin/openhouse/tables/config/MainApplicationConfig.java
Show resolved
Hide resolved
...les/src/main/java/com/linkedin/openhouse/tables/repository/impl/InternalRepositoryUtils.java
Outdated
Show resolved
Hide resolved
...main/java/com/linkedin/openhouse/tables/repository/impl/OpenHouseInternalRepositoryImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly have interface questions for a few current code blocks but they don't have to be blocking this change.
Summary
❗❗ The volume of this PR might be intimidating, please read through the description to make reviewing easier.
The primary goal of this PR is to remove
FsStorageProvider
from:services:tables
(and additionally:tables-test-fixture
)To achieve this goal, we do:
1: Add additional methods to
StorageClient
interface2: Add additional method to
FileIOManager
to reverse lookup a Storage for a FileIO3: Map FsStorageProvider methods to StorageManager
4: Fix log for StorageType
👆 thats it
Is this the end state of refactoring? NO. The goal of the PR is limited to removing
fsStorageProvider
. In future PRs we'll address CTAS, fileSecurer, stripPrefix, defaultTableLocation etc, currently these have been tagged withTODO
Changes
For all the boxes checked, please include additional details of the changes made in this pull request.
Testing Done
Additional Information
- remove fsStorageProvider
- Refactor: Remove @LegacyFileIO and Make DelegationRefreshToken use new cluster.yaml #100