Skip building remote compaction output file when not OK #13183
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jaykorean
force-pushed
the
skip_building_output_when_non_ok
branch
from
f3879eb to
81ec80a
Compare
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jaykorean merged this pull request in d46d1fa. |
anand1976
pushed a commit
that referenced
this pull request
Dec 5, 2024
Summary: During `FinishCompactionOutputFile()` if there's an IOError, we may end up having the output in memory, but table properties are not populated, because `outputs.UpdateTableProperties();` is called only when `s.ok()` is true. However, during remote compaction result serialization, we always try to access the `table_properties` which may be null. This was causing a segfault. We can skip building the output files in the result completely if the status is not ok. # Unit Test New test added ``` ./compaction_service_test --gtest_filter="*CompactionOutputFileIOError*" ``` Before the fix ``` Received signal 11 (Segmentation fault) Invoking GDB for stack trace... #4 0x00000000004708ed in rocksdb::TableProperties::TableProperties (this=0x7fae070fb4e8) at ./include/rocksdb/table_properties.h:212 212 struct TableProperties { #5 0x00007fae0b195b9e in rocksdb::CompactionServiceOutputFile::CompactionServiceOutputFile (this=0x7fae070fb400, name=..., smallest=0, largest=0, _smallest_internal_key=..., _largest_internal_key=..., _oldest_ancester_time=1733335023, _file_creation_time=1733335026, _epoch_number=1, _file_checksum=..., _file_checksum_func_name=..., _paranoid_hash=0, _marked_for_compaction=false, _unique_id=..., _table_properties=...) at ./db/compaction/compaction_job.h:450 450 table_properties(_table_properties) {} ``` After the fix ``` [ RUN ] CompactionServiceTest.CompactionOutputFileIOError [ OK ] CompactionServiceTest.CompactionOutputFileIOError (4499 ms) [----------] 1 test from CompactionServiceTest (4499 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (4499 ms total) [ PASSED ] 1 test. ``` Pull Request resolved: #13183 Reviewed By: anand1976 Differential Revision: D66770876 Pulled By: jaykorean fbshipit-source-id: 63df7c2786ce0353f38a93e493ae4e7b591f4ed9
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
During
FinishCompactionOutputFile()if there's an IOError, we may end up having the output in memory, but table properties are not populated, becauseoutputs.UpdateTableProperties();is called only whens.ok()is true.However, during remote compaction result serialization, we always try to access the
table_propertieswhich may be null. This was causing a segfault.We can skip building the output files in the result completely if the status is not ok.
Unit Test
New test added
Before the fix
After the fix