Conversation
Summary of ChangesHello @dmga44, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's chunk locking mechanism by introducing chunkserver-side locking. This change aims to improve the robustness and consistency of write and truncate operations by allowing chunkservers to track the 'being written' state of chunk parts. The master now actively manages these locks, ensuring that operations are properly serialized and that chunk versions are correctly increased in the event of failures or disconnections. This provides a more reliable foundation for handling concurrent modifications to chunks across the distributed file system. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature: chunkserver-side chunk locking. The changes are extensive, touching the master, chunkserver, and network protocol, but they appear to be consistent and correct. The refactoring of chunk_multi_modify and chunk_multi_truncate in src/master/chunks.cc is a major improvement for code readability and maintainability. The new locking mechanism, including deferred job execution on locked chunks and proper handling of disconnections and errors, is thoughtfully designed.
I have a couple of suggestions for minor improvements. One is to improve efficiency in chunk_handle_disconnected_copies by reducing redundant iterations. The other is to replace a magic number with a named constant for better readability.
Overall, this is a high-quality contribution.
There was a problem hiding this comment.
Pull request overview
This pull request implements chunkserver-side chunk locking to improve write operation tracking and consistency. The system now tracks whether chunk parts are being written on the chunkserver side, in addition to the existing client-side locking mechanism. This allows the master to make better decisions about chunk availability and operations.
Changes:
- Introduced new protocol messages for chunkserver-side chunk locking (lock, unlock, write end status)
- Refactored chunk modification operations in the master into separate, well-documented functions
- Implemented chunk lock job management in the chunkserver's JobPool
- Modified client-side write operations to always communicate with chunkservers for lock release
- Fixed test_read_corrupted_files test by adding WriteMaxRetries configuration
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/protocol/matocs.h | Added packet definitions for createAndLockChunk, setVersionAndLock, chunkLock, chunkUnlock, and duplicateAndLockChunk |
| src/protocol/cstoma.h | Added packet definitions for chunkLock and writeEndStatus responses |
| src/protocol/SFSCommunication.h | Added packet command constants for the new lock-related messages |
| src/common/saunafs_version.h | Added version constant for chunkserver-side chunk lock feature (5.7.0) |
| src/common/event_loop.h | Fixed documentation comment for eventloop_time() (returns seconds, not milliseconds) |
| src/master/matocsserv.h | Updated function signatures to support lock tracking with sentChunkLock parameter |
| src/master/matocsserv.cc | Implemented new message sending functions with version checks for backward compatibility |
| src/master/chunks.h | Added declarations for new chunk operation functions and lock status handlers |
| src/master/chunks.cc | Major refactoring: extracted chunk operations into separate functions, added beingWritten tracking, improved disconnection handling |
| src/master/filesystem_operations.cc | Updated changelog to use operation type instead of boolean flag |
| src/chunkserver/masterconn.h | Added declaration for masterconn_get_job_pool() |
| src/chunkserver/masterconn.cc | Implemented masterconn_get_job_pool() to expose job pool |
| src/chunkserver/master_connection.h | Added declarations for new lock-related message handlers |
| src/chunkserver/master_connection.cc | Implemented handlers for lock messages and lock-aware job completion callbacks |
| src/chunkserver/bgjobs.h | Added chunk lock management API to JobPool |
| src/chunkserver/bgjobs.cc | Implemented chunk lock tracking and deferred job execution for locked chunks |
| src/chunkserver/chunk_high_level_ops.cc | Integrated lock enforcement and release into write operations |
| src/chunkserver/chunkserver_entry.cc | Added defensive checks before calling cleanup() |
| src/mount/writedata.cc | Removed optimization to always communicate with chunkservers for lock release |
| tests/test_suites/ShortSystemTests/test_read_corrupted_files.sh | Added WriteMaxRetries=2 configuration for test reliability |
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
888d7ec to
09b15e5
Compare
|
@gemini-code-assist Please review again. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature for chunkserver-side chunk locking. The changes are comprehensive, spanning the master, chunkserver, and client-side components, and appear to be correctly implemented.
The most notable improvement is the major refactoring of chunk_multi_modify and chunk_multi_truncate functions in src/master/chunks.cc. Breaking down these large functions into smaller, single-purpose, and well-documented functions (chunk_create_operation, chunk_increase_version_operation, etc.) greatly enhances the readability and maintainability of the code.
The new locking logic seems robust, with careful attention to concurrency, potential deadlocks, and error handling scenarios such as chunkserver disconnections during a write operation. The state management on both the master (with the beingWritten flag) and the chunkserver (via the JobPool) is consistent and appears correct.
Overall, this is a high-quality contribution that not only adds an important feature but also improves the existing codebase. The code is clean, well-documented, and demonstrates a solid understanding of the system's architecture.
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
09b15e5 to
0c1055b
Compare
|
@gemini-code-assist Please review again. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature: chunkserver-side chunk locking. This enhances the existing client-side locking mechanism by allowing the master to track writes at the chunkserver level, improving consistency. The implementation involves new packet types for lock management, updates to the chunkserver's JobPool to handle locked chunks and pending jobs, and corresponding changes on the master side to manage the lock lifecycle.
The review identified a potential crash in a new unit test due to a nullptr being passed to a callback that doesn't handle it. Additionally, there are a couple of opportunities to reduce code duplication in the new JobPool logic by extracting common functionality into helper methods.
Overall, the changes are well-implemented. The extensive refactoring of chunk_multi_modify and chunk_multi_truncate in src/master/chunks.cc is a notable improvement, making the code much more readable and maintainable.
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
0c1055b to
342ed99
Compare
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
2 times, most recently
from
1202734 to
b8b0dd0
Compare
dmga44
requested review from
antuan96314,
lgsilva3087,
ralcolea,
rolysr and
uristdwarf
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
5 times, most recently
from
2651c75 to
004dada
Compare
|
@gemini-code-assist Please review again. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature: chunkserver-side locking. The implementation is comprehensive, touching on the master, chunkserver, and client components. The refactoring of chunk_multi_modify and chunk_multi_truncate into smaller, more focused functions is a major improvement for code clarity and maintainability. The new locking logic appears robust, and the addition of tests, especially the enhanced concurrent write test that toggles the feature flag, is excellent. I've identified one potential race condition related to client lock timeouts, which I've detailed in a specific comment. Overall, this is a high-quality contribution.
tests/test_suites/ShortSystemTests/test_concurrent_random_writes_on_chunk.sh
Outdated
Show resolved
Hide resolved
tests/test_suites/ShortSystemTests/test_concurrent_random_writes_on_chunk.sh
Outdated
Show resolved
Hide resolved
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
004dada to
a7009ec
Compare
|
@gemini-code-assist Please review again. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature: chunkserver-side chunk locking. The changes are extensive, touching the master, chunkserver, client-side mount, and protocol definitions, but they are logically sound and consistent with the detailed description provided.
The core of the change is to allow the master to track writes not just by client acknowledgements but also by chunkserver-side status, which improves the robustness of chunk locking. This is controlled by a new, reloadable configuration option USE_CHUNKSERVER_SIDE_CHUNK_LOCK.
Key highlights of this PR include:
- A major and beneficial refactoring of the
chunk_multi_modifyandchunk_multi_truncatefunctions in the master, which greatly improves readability and maintainability. - Introduction of new packets and handlers for lock management between the master and chunkservers, with proper handling for backward compatibility.
- Robust implementation on the chunkserver side to queue jobs on locked chunks and manage the lock lifecycle.
- Thoughtful updates to the client-side write logic to ensure lock release protocols are followed.
- Comprehensive updates to unit tests and system tests, including a stress test that toggles the new feature dynamically.
The code quality is high, with good attention to detail, especially regarding memory management of network packets and handling of concurrent operations. I have reviewed the changes carefully and found no issues that meet the required severity levels. This is an excellent contribution to the project.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/mount/writedata.cc:623
- At line 618, when
e.status() == SAUNAFS_ERROR_LOCKED, the retry counter is not incremented and the error is thrown again (line 623). However, there's no mechanism shown here to wait or backoff before retrying. If the chunk remains locked, this could lead to a tight retry loop consuming CPU. Consider whether a delay or backoff mechanism should be added before retrying locked chunks, or verify that such a mechanism exists elsewhere in the write job scheduling logic.
if (inodeData_->trycnt >= maxretries && e.status() != SAUNAFS_ERROR_LOCKED) {
// Convert error to an unrecoverable error
throw UnrecoverableWriteException(e.message(), e.status());
} else {
// This may be recoverable or unrecoverable error
throw;
tests/test_suites/ShortSystemTests/test_concurrent_random_writes_on_chunk.sh
Show resolved
Hide resolved
tests/test_suites/ShortSystemTests/test_concurrent_random_writes_on_chunk.sh
Show resolved
Hide resolved
lgsilva3087
left a comment
lgsilva3087
left a comment
There was a problem hiding this comment.
Great work. Please check my comments.
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
a7009ec to
1b04041
Compare
|
Great work @dmga44!! 👍 💪 🔥 |
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
1b04041 to
17d1d2c
Compare
The current implementation of the chunk locking in the system relies only in the client responses when finishing write and truncate operations. This commit targets including also chunkserver side replies after receiving an actual stream of write operations to decide whether the chunk is locked or not. This is not useful in the current state of code and must not affect the behavior of the system because the client responses to the master in order to unlock the chunk happen after the data is already in the chunkserver or there is some error. The idea of the change is to keep track in each of the chunk parts of whether it is expected to be written at the time. Please note this affects all the writes coming from the client and some truncates. Those cases trigger one these functions: - chunk_create_operation - chunk_increase_version_operation - chunk_lock_operation - chunk_duplicate_operation which can be traced back to the chunk_multi_modify function. After checking some conditions (specially if the CS version supports this feature) the new packets arrive to the CS and in most of the cases the write operation needs to wait for its responses, and the chunk parts are considered being written. In the CS side, the locking is handled by the master's main JobPool. It can start, enforce, end and erase chunk lock jobs. The locked chunks are special the way that after ending the write operations on it, the master receives the status of the write operations that was not told to the clients, so far it is always OK because everything is told to the clients. Master jobs on enforced locked chunks will have to wait until the chunk is released, i.e finish writing. Back again in the master side, it handles disconnection of parts being written and errors on those writes the same way: increasing version when the chance appears. Side changes: - refactor chunk_multi_modify and chunk_multi_truncate functions. - change the effective grace time when starting and trying to create chunk before responding "no space" from 600s to 60s. - add documentation to some members of the Chunk class deeply involved in the change. - in the client side, always talk to the chunkservers when writing. - the main SaunaFS package version was updated to 5.8.0 Signed-off-by: Dave <dave@leil.io>
The new behavior implemented previously can be now enabled and disabled via the option USE_CHUNKSERVER_SIDE_CHUNK_LOCK. This option is reloadable. The decision making instant is at the moment of sending the specific packet type to the chunkservers. The testing framework was modified in order to enable this option in all tests, while the default master behavior has it disabled by default. Signed-off-by: Dave <dave@leil.io>
Current implementation has a critical error when the following conditions are met: - clients are creating files in goal ec(D, P). - more than one minute has passed. - all the chunkservers restart. In such case, it is very likely that master replies to some of the chunk write (creations) requests SAUNAFS_ERROR_NOSPACE. This happens when not being able to create the chunk due to not having at least D chunkservers available and having at least one available. The error code SAUNAFS_ERROR_NOSPACE is not retryable by the client, which is correct, but makes the write operations to completely fail. The incorrect behavior is the one from the master, which must reply SAUNAFS_ERROR_NOCHUNKSERVERS in such cases and only leave the NOSPACE error code for the case when at least D chunkservers are available. Signed-off-by: Dave <dave@leil.io>
dmga44
force-pushed
the
feat-cs-size-chunk-lock
branch
from
17d1d2c to
4f0aea4
Compare
The current implementation of the chunk locking in the system relies only in the client responses when finishing write and truncate operations. This commit targets including also chunkserver side replies after receiving an actual stream of write operations to decide whether the chunk is locked or not. This is not useful in the current state of code and must not affect the behavior of the system because the client responses to the master in order to unlock the chunk happen after the data is already in the chunkserver or there is some error.
The idea of the change is to keep track in each of the chunk parts of whether it is expected to be written at the time. Please note this affects all the writes coming from the client and some truncates. Those cases trigger one these functions:
which can be traced back to the chunk_multi_modify function. After checking some conditions (specially if the CS version supports this feature) the new packets arrive to the CS and in most of the cases the write operation needs to wait for its responses, and the chunk parts are considered being written.
In the CS side, the locking is handled by the master's main JobPool. It can start, enforce, end and erase chunk lock jobs. The locked chunks are special the way that after ending the write operations on it, the master receives the status of the write operations that was not told to the clients, so far it is always OK because everything is told to the clients. Master jobs on enforced locked chunks will have to wait until the chunk is released, i.e finish writing.
Back again in the master side, it handles disconnection of parts being written and errors on those writes the same way: increasing version when the chance appears.
Side changes:
chunk before responding "no space" from 600s to 60s.
in the change.
usedummylockidfromchunk_multi_modifyandwriteChunkfunctions.
were improved.
The new behavior implemented previously can be now enabled and disabled
via the option USE_CHUNKSERVER_SIDE_CHUNK_LOCK. This option is
reloadable. The decision making instant is at the moment of sending the
specific packet type to the chunkservers.
The testing framework was modified in order to enable this option in
all tests, while the default master behavior has it disabled.
Closes LS-314.
Signed-off-by: Dave dave@leil.io