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

Initial implementation of ExternalTableBuilder #13434

Closed
wants to merge 4 commits into from

Conversation

anand1976
Copy link
Contributor

This PR adds the ability to use an ExternalTableBuilder through the SstFileWriter to create external tables. This is a counterpart to #13401 , which adds the ExternalTableReader. The support for external tables is confined to ingestion only DBs, with external table files ingested into the bottommost level only. #13431 enforces ingestion only DBs by adding a disallow_memtable_writes column family option.

Test plan:
New unit tests in table_test.cc

@facebook-github-bot
Copy link
Contributor

@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@anand1976 anand1976 force-pushed the external_table_writer branch from 5030429 to 3bd4daa Compare March 4, 2025 18:26
@facebook-github-bot
Copy link
Contributor

@anand1976 has updated the pull request. You must reimport the pull request before landing.

@anand1976 anand1976 requested a review from jaykorean March 5, 2025 05:42
Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

Overall looks good. I think I would slightly prefer "custom table reader/writer" or "user table reader/write" to "external" because the files are managed internally by RocksDB. The format or schema is custom / user-defined.

// Flush and close the table file
virtual Status Finish() = 0;

// Delete the partial file and release any allocated resources. Either this
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the internal builders do not delete the file on abandon, presumably to run the file through throttled deletion. Can you explain that difference?

Btw it seems like the dtor should just take care of whatever cleanup with no need for this function. Not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In a running db, the file is deleted in the background. But the SstFileWriter flow seems a bit inconsistent. If there is an error in Finish(), it deletes the file. But if the user just forgets to call Finish(), the file is not deleted.

I'll just keep Abandon() for now. The external table builder will only be used by SstFileWriter, and in that context deleting the file on abandon seems like the right thing to do.

@anand1976 anand1976 force-pushed the external_table_writer branch from 3bd4daa to da9012a Compare March 5, 2025 22:40
@facebook-github-bot
Copy link
Contributor

@anand1976 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@anand1976 merged this pull request in 14c949d.

anand1976 added a commit that referenced this pull request Mar 6, 2025
Summary:
This PR adds the ability to use an ExternalTableBuilder through the SstFileWriter to create external tables. This is a counterpart to #13401 , which adds the ExternalTableReader. The support for external tables is confined to ingestion only DBs, with external table files ingested into the bottommost level only. #13431 enforces ingestion only DBs by adding a disallow_memtable_writes column family option.

Pull Request resolved: #13434

Test Plan: New unit tests in table_test.cc

Reviewed By: pdillinger

Differential Revision: D70532054

Pulled By: anand1976

fbshipit-source-id: a837487eadfabed9627a0eceb403bfc5fc2c427c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants