Skip to content

refactor: Throw exception rather than LOG(FATAL) when encounter IO error#555

Merged
zhanglei1949 merged 6 commits into
alibaba:mainfrom
zhanglei1949:zl/throw-exp-wal-recover
Jun 17, 2026
Merged

refactor: Throw exception rather than LOG(FATAL) when encounter IO error#555
zhanglei1949 merged 6 commits into
alibaba:mainfrom
zhanglei1949:zl/throw-exp-wal-recover

Conversation

@zhanglei1949

Copy link
Copy Markdown
Member

Fix #514

@zhanglei1949 zhanglei1949 requested review from Copilot and liulx20 June 16, 2026 03:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR updates LocalWalParser to throw IO exceptions instead of terminating the process on WAL file I/O failures, and adds unit tests to validate these error paths (Fix #514).

Changes:

  • Replace LOG(FATAL) behavior with IOException throws for open()/mmap() failures in LocalWalParser.
  • Ensure LocalWalParser::open() resets prior state via close() and make close() fully clear internal state.
  • Add new LocalWalParser unit tests for valid parsing, unreadable file errors, mmap failures, and empty directories.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
tests/transaction/test_insert_transaction.cc Adds LocalWalParser tests (success + IO failure cases) and supporting helpers to create WAL files.
src/transaction/wal/local_wal_parser.cc Switches fatal logging to exceptions for I/O errors; resets parser state on open/close.
Comments suppressed due to low confidence (1)

src/transaction/wal/local_wal_parser.cc:1

  • close() is called before throwing on open()/mmap() failure. If THROW_IO_EXCEPTION (or the exception ctor) relies on errno, close()/::close() can overwrite errno, producing incorrect diagnostics. Capture errno immediately after the failing syscall (before any cleanup) and include it in the thrown exception (or pass it into the exception) so the error reason is accurate.
/** Copyright 2020 Alibaba Group Holding Limited.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/transaction/test_insert_transaction.cc
Comment thread tests/transaction/test_insert_transaction.cc
Comment thread tests/transaction/test_insert_transaction.cc
Comment thread src/transaction/wal/local_wal_parser.cc Outdated
void* mmapped_buffer = mmap(NULL, file_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (fd == -1) {
close();
THROW_IO_EXCEPTION("Failed to open wal file: " + path);

@liulx20 liulx20 Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we expose error information using operations like strerror(errno)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

resolved.

@zhanglei1949 zhanglei1949 merged commit 4c44ed7 into alibaba:main Jun 17, 2026
22 checks passed
longbinlai pushed a commit to longbinlai/neug that referenced this pull request Jun 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] mmap fails during WAL recovery - process aborts via LOG(FATAL)

3 participants