Skip to content

refactor: Disable auto checkpoint for read-only mode#237

Merged
zhanglei1949 merged 9 commits into
alibaba:mainfrom
zhanglei1949:zl/no-dump-readonly
Apr 17, 2026
Merged

refactor: Disable auto checkpoint for read-only mode#237
zhanglei1949 merged 9 commits into
alibaba:mainfrom
zhanglei1949:zl/no-dump-readonly

Conversation

@zhanglei1949
Copy link
Copy Markdown
Member

@zhanglei1949 zhanglei1949 commented Apr 16, 2026

When the database is opened in read-only mode, there is no need for us to auto checkpoint on close.

Fix #239
Fix #240

@zhanglei1949 zhanglei1949 requested a review from liulx20 April 16, 2026 07:21
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors database shutdown behavior so that automatic checkpoint creation is skipped when the database is opened in read-only mode, avoiding unnecessary persistence work (and potential writes) on close.

Changes:

  • Gate createCheckpoint() on DBMode::READ_WRITE in NeugDB::Close(), and add a VLOG entry when checkpointing occurs.
  • Remove unused _move_data helper templates from id_indexer.h.
  • Adjust closed_ stores in NeugDB::Close() (removes one redundant store; adds another at the end).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/main/neug_db.cc Skip checkpoint-on-close for read-only mode; add logging; tweak closed_ updates.
include/neug/utils/id_indexer.h Remove unused helper templates at end of header.

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

Comment thread src/main/neug_db.cc Outdated
@zhanglei1949 zhanglei1949 requested a review from longbinlai April 16, 2026 07:53
@zhanglei1949 zhanglei1949 requested a review from Copilot April 16, 2026 09:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

src/server/neug_db_session.cc:108

  • The term 'read-only mode' is used for both the database configuration (DBMode::READ_ONLY) and the session access mode (AccessMode::kRead). The second message ('Read-only mode does not support...') can be confused with the DB-level message above. Consider clarifying the access-mode message (e.g., 'Access mode READ does not support write operations') to make client-facing errors unambiguous.
      return neug::Status(
          neug::StatusCode::ERR_INVALID_ARGUMENT,
          "Database is in read-only mode; write operations are not allowed.");
    }
  }
  if (mode == neug::AccessMode::kRead) {
    if (!is_read_only(flags)) {
      return neug::Status(neug::StatusCode::ERR_INVALID_ARGUMENT,
                          "Read-only mode does not support write operations.");

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

Comment thread tools/python_bind/tests/test_tp_service.py
Comment thread tools/python_bind/tests/test_tp_service.py
Comment thread tools/python_bind/tests/test_tp_service.py Outdated
Comment thread tools/python_bind/tests/test_tp_service.py
Comment thread tools/python_bind/tests/test_tp_service.py
Comment thread src/server/neug_db_session.cc
@zhanglei1949 zhanglei1949 merged commit 1858f88 into alibaba:main Apr 17, 2026
17 checks passed
@lnfjpt lnfjpt mentioned this pull request May 26, 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.

Read-only database incorrectly accepts insert/update requests via HTTP service When closing a read-only database, we should never auto-checkpoint

3 participants