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

Add storageType field to HouseTable Entity #81

Merged
merged 7 commits into from May 7, 2024
Merged

Add storageType field to HouseTable Entity #81

merged 7 commits into from May 7, 2024

Conversation

ctrezzo
Copy link
Contributor

@ctrezzo ctrezzo commented Apr 22, 2024 •
edited

Summary

Issue Added a storageType field to the HouseTable Entity to support multiple storage types.

Resolves #80

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

For all the boxes checked, please include additional details of the changes made in this pull request.

Added storageType field to the HouseTable DTO.

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

@ctrezzo ctrezzo changed the title Add storageType field to HouseTable Entity #80 Add storageType field to HouseTable Entity Apr 22, 2024
jainlavina
jainlavina reviewed Apr 22, 2024
View reviewed changes
Copy link
Collaborator

@jainlavina jainlavina left a comment

Choose a reason for hiding this comment

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

We should add a test to check backwards compatibility. Check that if storageType is not populated in the database then it does not break anything.

HotSushi
HotSushi requested changes Apr 23, 2024
View reviewed changes
Copy link
Collaborator

@HotSushi HotSushi left a comment

Choose a reason for hiding this comment

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

the changes are looking good overall, ty. Its only missing populating the field in HouseTable entity. Could we change HouseTableMapper::toHouseTable to populate storageType="hdfs" if it's not already set in tableProperties?

Also, a local docker test would be fantastic to have: https://github.com/linkedin/openhouse/blob/main/SETUP.md

@ctrezzo
Copy link
Contributor Author

ctrezzo commented May 3, 2024 •
edited

Hey @HotSushi and @jainlavina! I have updated the pull request. Please take a look. Here are the main changes:

  1. storageType field is now populated with the HDFS default
  2. I added a unit test to validate defaults are set in the HouseTable builder properly
  3. I did a manual test with docker containers where I created a table via a spark shell and verified that the user_table_row table had the new row with "hdfs" set as the value for the storage_type column.
  4. All unit tests pass.

HotSushi
HotSushi reviewed May 3, 2024
View reviewed changes
Copy link
Collaborator

@HotSushi HotSushi left a comment

Choose a reason for hiding this comment

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

Its looking good, can we add a screenshot of the request received in HTS service, maybe from the docker log?

...ernalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/model/HouseTableTest.java Show resolved Hide resolved
@ctrezzo
Copy link
Contributor Author

ctrezzo commented May 3, 2024

@HotSushi Attached are the screenshots from the docker log during the manual test. Thanks!

Screenshot 2024-05-03 at 2 03 50 PM Screenshot 2024-05-03 at 2 03 23 PM Screenshot 2024-05-03 at 2 00 31 PM

@ctrezzo
Copy link
Contributor Author

ctrezzo commented May 6, 2024

@HotSushi Attaching screenshot from HTS as well.

Screenshot 2024-05-06 at 12 46 02 PM

@ctrezzo
Copy link
Contributor Author

ctrezzo commented May 6, 2024

Taking a look at the failed test

HotSushi
HotSushi approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@HotSushi HotSushi left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the extensive testing in this PR, unit tests and docker screenshots made it very easy to review

@HotSushi HotSushi merged commit d8d136b into linkedin:main May 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jainlavina jainlavina jainlavina left review comments

@HotSushi HotSushi HotSushi approved these changes

@autumnust autumnust Awaiting requested review from autumnust

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add storageType field to HouseTable Entity
3 participants
@ctrezzo @HotSushi @jainlavina