fix: unsafe method panic, reduce log, slice work with id #77
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
medcl
reviewed
Feb 8, 2025
modules/queue/disk_queue/module.go
Outdated
| //log.Debugf("save offset: %v", offset.EncodeToString()) | ||
|
|
||
| err = kv.AddValue(ConsumerOffsetBucket, util.UnsafeStringToBytes(getCommitKey(k, consumer)), []byte(offset.EncodeToString())) | ||
| err = kv.AddValue(ConsumerOffsetBucket, util.UnsafeGetStringToBytes(getCommitKey(k, consumer)), []byte(offset.EncodeToString())) |
There was a problem hiding this comment.
Why not fix UnsafeStringToBytes rather create another function with same behavior, i don't think it is necessary, and the output of getCommitKey should not be empty, if empty you should call panic, i don't really get the case that you can fix with the change.
There was a problem hiding this comment.
when getCommitKey not empty, the return []byte just has len , but cap=0, so this k.AddValue will panic
silenceqi
approved these changes
Feb 12, 2025
medcl
approved these changes
Feb 13, 2025
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.
What does this PR do
This pull request includes several updates and improvements across multiple files. The changes primarily involve enhancing logging messages for better clarity, adding error handling, and updating function implementations. Below are the most important changes grouped by theme:
Logging Improvements:
modules/queue/disk_queue/consumer.goto include more detailed information, such asworkerID. This improves traceability and debugging. [1] [2] [3] [4] [5] [6]plugins/elastic/bulk_indexing/bulk_indexing.goto provide more context by including theworkerIDin log messages. This helps in identifying the specific worker handling the task. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Error Handling:
modules/queue/disk_queue/consumer.goto provide more detailed error messages when encountering issues with message sizes. [1] [2]Function Implementations:
UnsafeGetStringToBytesincore/util/bytes.goto handle empty string cases and avoid nil slices. This function provides a safer alternative for converting strings to bytes.UnsafeStringToByteswithUnsafeGetStringToBytesinmodules/queue/disk_queue/module.goto ensure the safer handling of string-to-byte conversions.Typographical Fixes:
getElasticsearchMeatadatatogetElasticsearchMetadatainplugins/elastic/bulk_indexing/bulk_indexing.go. [1] [2]These changes collectively improve the robustness, clarity, and maintainability of the codebase.
Rationale for this change
Standards checklist