feat(s3stream): lock scope optimization and persistence settings encapsulation #1715
Conversation
CLFutureX
requested review from
superhx,
SCNieh,
ShadowySpirits and
Chillax-0v0
as code owners
|
In the future, the WAL plan will be changed to a sequential success model to improve code maintainability while meeting current performance requirements. This PR increases the consideration cost for concurrent scenarios, and it is not recommended to over-optimize in scenarios without performance issues. |
Yeah, I agree with your point. But for this PR specifically, the performance improvement is indeed an added bonus. I think what's more significant is the improvement in code maintainability achieved by cohesively incorporating the persistence behavior into the object itself and decoupling the persistence operations from the ack response handling. |
CLFutureX
changed the title
Well, tactually for this PR, the performance improvement is more of an add-on. I think what's more important is to cohesively incorporate the persistence behavior into the object itself and separate the operations.
Hey, I've given it some thought and made further optimizations. I've moved the persistence operation back into the 'after' method, which indeed makes more sense. Additionally, I mainly adjusted the scope of the locking. |
CLFutureX
changed the title
# Conflicts: # s3stream/src/main/java/com/automq/stream/s3/S3Storage.java
| } | ||
| for (WalWriteRequest waitingAckRequest : waitingAckRequests) { | ||
| waitingAckRequest.record.retain(); | ||
| boolean full = deltaWALCache.put(waitingAckRequest.record); |
There was a problem hiding this comment.
Currently, LogCache relies on external guarantees to put individual Stream Records in order, moving the lock to WALCallbackSequencer#after will have thread concurrency safety issues.
I have created an #1718 about optimizing WAL sequential callbacks. It would be the first step to make the AutoMQ storage layer code more concise. If you are interested, I would greatly appreciate it if you would take it. Next, we can continue to optimize the concurrency issues in S3Storage. |
That's great, I'd be happy to take it |
issue