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

feat(logstream): deprecate error field in AppendResult for atomic append #985

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ijsong
Copy link
Member

@ijsong ijsong commented Feb 7, 2025

What this PR does

This commit deprecates the error field in AppendResult as a step towards
implementing atomic append operations while maintaining backward compatibility.

Previously, the Append RPC could return partial success/failure results, with
some log entries in a batch being appended successfully and others failing. This
was indicated by the error field in AppendResult.

To support atomic append operations without breaking existing clients, the error
field is deprecated instead of being removed completely. This change allows
clients to continue using the error field for now, but they should be aware that
it will be removed in a future release. Clients should start migrating to the
new atomic append API as soon as possible.

The following changes were made to deprecate the error field:

  • The error field in AppendResult is marked as deprecated in the protobuf
    definition.
  • The Append RPC implementation no longer populates the error field.

The next step is to implement atomic append operations in the client API. This
will enable clients to append multiple log entries atomically, which will help
to resolve #843.

@ijsong ijsong requested a review from hungryjang as a code owner February 7, 2025 05:39
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.07%. Comparing base (30645f0) to head (48dd48a).

Files with missing lines Patch % Lines
internal/storagenode/log_server.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #985      +/-   ##
==========================================
+ Coverage   79.91%   80.07%   +0.16%     
==========================================
  Files         178      178              
  Lines       21408    21412       +4     
==========================================
+ Hits        17108    17146      +38     
+ Misses       3504     3485      -19     
+ Partials      796      781      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from 64e44ac to bd370e3 Compare February 18, 2025 23:54
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from 5a03f0d to 0afc4ab Compare February 18, 2025 23:54
@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from bd370e3 to fdd3cd2 Compare February 19, 2025 01:15
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from 0afc4ab to 4511a81 Compare February 19, 2025 01:15
@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from fdd3cd2 to 3785ebc Compare February 21, 2025 09:00
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from 4511a81 to f172616 Compare February 21, 2025 09:00
@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from 3785ebc to 218206a Compare February 21, 2025 10:16
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from f172616 to c119d34 Compare February 21, 2025 10:16
@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from 218206a to be0ae21 Compare February 21, 2025 12:59
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from c119d34 to b3d4566 Compare February 21, 2025 12:59
@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from be0ae21 to 7bbd20c Compare February 21, 2025 13:20
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from b3d4566 to 0515210 Compare February 21, 2025 13:20
@ijsong ijsong force-pushed the commit_wait_task_for_append_batch branch from 7bbd20c to 38bffc0 Compare February 21, 2025 13:42
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from 0515210 to b8ded9d Compare February 21, 2025 13:42
Base automatically changed from commit_wait_task_for_append_batch to main February 21, 2025 13:54
This commit deprecates the error field in AppendResult as a step towards
implementing atomic append operations while maintaining backward compatibility.

Previously, the Append RPC could return partial success/failure results, with
some log entries in a batch being appended successfully and others failing. This
was indicated by the error field in AppendResult.

To support atomic append operations without breaking existing clients, the error
field is deprecated instead of being removed completely. This change allows
clients to continue using the error field for now, but they should be aware that
it will be removed in a future release. Clients should start migrating to the
new atomic append API as soon as possible.

The following changes were made to deprecate the error field:

- The error field in AppendResult is marked as deprecated in the protobuf
  definition.
- The Append RPC implementation no longer populates the error field.

The next step is to implement atomic append operations in the client API. This
will enable clients to append multiple log entries atomically, which will help
to resolve #843.
@ijsong ijsong force-pushed the deprecate_error_field_in_appendresult branch from b8ded9d to 48dd48a Compare February 21, 2025 13:55
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.

Implement atomic append operation
1 participant