Skip to content

[ENH] Add log sealing to the go service. #4554

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rescrv
Copy link
Contributor

@rescrv rescrv commented May 15, 2025

Description of changes

This PR does the following:

  • Adds a migration to add the is_sealed column to the collection table
    in the log database.
  • Updates the go/ README.md to mention the correct atlas command to run.
  • Updates the go store queries to recognize the new field.
  • Add a new SealLog query to unconditionally seal a log.
  • Propagate the is_sealed column from table to IsSealed field in the
    protobuf message.
  • Add a test that push logs will fail when sealed.
  • Add support to the RustLogService for the new IDL.
  • Translate is_sealed responses into explicit errors in the log
    client.

Test plan

CI, as usual.

Documentation Changes

N/A

This PR does the following:
- Adds a migration to add the `is_sealed` column to the collection table
  in the log database.
- Updates the go/ README.md to mention the correct atlas command to run.
- Updates the go store queries to recognize the new field.
- Add a new SealLog query to unconditionally seal a log.
- Propagate the `is_sealed` column from table to `IsSealed` field in the
  protobuf message.
- Add a test that push logs will fail when sealed.
- Add support to the RustLogService for the new IDL.
- Translate `is_sealed` responses into explicit errors in the log
  client.
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Contributor

propel-code-bot bot commented May 15, 2025

Add Log Sealing Functionality to Go Log Service

This PR implements log sealing capabilities to prevent further writes to logs. It adds an is_sealed column to the collection table in the log database, along with corresponding API changes to propagate the sealed status from database to client. The implementation includes schema migration, updated queries, client-side error handling in Rust, and comprehensive test coverage.

Key Changes:
• Added is_sealed boolean column to collection table with migration script
• Added SealLog query to mark a log as sealed
• Updated log client to recognize and handle sealed logs
• Modified API responses to include sealed status information

Affected Areas:
• Database schema (collection table)
• Go log service and repository
• Rust log client
• Protocol buffers API definitions

This summary was automatically generated by @propel-code-bot

@rescrv rescrv changed the title [ENH] Add an log sealing to the go service. [ENH] Add log sealing to the go service. May 15, 2025
@rescrv rescrv requested review from codetheweb and Copilot May 15, 2025 15:40
Copy link

@Copilot 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

This PR adds log sealing functionality to prevent writes to sealed logs by propagating a new is_sealed flag through the database, gRPC service, and client API.

  • Adds a migration and schema changes to include an is_sealed column
  • Updates queries, protocol buffers, and error handling to support sealing logs
  • Extends tests and documentation to cover the new log sealing behavior

Reviewed Changes

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

Show a summary per file
File Description
rust/log/src/grpc_log.rs Adds a new enum variant and response logic for sealed logs
rust/log-service/src/lib.rs Modifies the SealLog response to include log_is_sealed
idl/chromadb/proto/logservice.proto Updates the protobuf message for log responses
go/pkg/log/store/schema/collection.sql Alters the collection table to add the is_sealed column
go/pkg/log/store/queries/queries.sql Introduces the SealLog query using an INSERT operation
go/pkg/log/store/migrations/* Adds a new migration file to modify the collection schema
go/pkg/log/store/db/queries.sql.go Updates sqlc queries to handle the is_sealed field
go/pkg/log/server/server.go Propagates the new is_sealed flag in PushLogs responses
go/pkg/log/repository/log_test.go Updates tests to check behavior when logs are sealed
go/pkg/log/repository/log.go Modifies InsertRecords to return and check the log sealing status
go/README.md Adds instructions for running schema migrations
Comments suppressed due to low confidence (1)

go/pkg/log/repository/log_test.go:119

  • The variable 'records' is used in the loop without being defined; ensure that it is initialized or the correct variable is used for the test input.
for i, record := range records {

@@ -64,3 +64,6 @@ INSERT INTO record_log ("offset", collection_id, timestamp, record)
SELECT record_log.offset, $2, record_log.timestamp, record_log.record
FROM record_log
WHERE record_log.collection_id = $1;

-- name: SealLog :one
INSERT INTO collection (id, is_sealed) values($1, true) returning *;
Copy link
Preview

Copilot AI May 15, 2025

Choose a reason for hiding this comment

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

The SealLog query uses an INSERT to change the is_sealed flag; if a collection already exists, this could lead to a duplicate key conflict. Consider using an UPDATE statement to set is_sealed to true for an existing collection.

Copilot uses AI. Check for mistakes.

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.

2 participants