feat(storagenode): replica in the learning state does not report to a metadata repository #159
Conversation
Codecov ReportBase: 63.03% // Head: 62.95% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
- Coverage 63.03% 62.95% -0.08%
==========================================
Files 126 126
Lines 17052 17061 +9
==========================================
- Hits 10748 10741 -7
- Misses 5754 5764 +10
- Partials 550 556 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -348,8 +348,11 @@ func (lse *Executor) Report(_ context.Context) (snpb.LogStreamUncommitReport, er | |||
| atomic.AddInt64(&lse.inflight, 1) | |||
| defer atomic.AddInt64(&lse.inflight, -1) | |||
|
|
|||
| if lse.esm.load() == executorStateClosed { | |||
| switch lse.esm.load() { | |||
There was a problem hiding this comment.
Is there any chance that lse.esm.load and lse.lsc.reportCommitBase will be in learning state between executions so incorrect information will be read?
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @hungryjang and @ijsong)
internal/storagenode/logstream/executor.go line 351 at r1 (raw file):
Previously, hungryjang (Hyungkeun Jang) wrote…
Is there any chance that
lse.esm.loadandlse.lsc.reportCommitBasewill be in learning state between executions so incorrect information will be read?
Good point. Currently, the only way to solve the concurrency issue is to use mutex. There is a good candidate who needs to change it as shared mutex:
varlog/internal/storagenode/logstream/executor.go
Lines 51 to 52 in b796fd7
However, it can affect the response time of the report during handling seal, unseal, or sync-related operations. Does the inaccurate information in the report affect the performance of the metadata repository?
… metadata repository Previously, a log stream replica in the learning state sent reports to the metadata repository, and the log stream replica had to use commit contexts sent from the source replica. However, the source cannot do that in a future patch because replicas in a log stream will store the latest commit context rather than all commit contexts. This patch slightly changes the report's behavior in a log stream replica, not to send it while in a learning state. When the log stream replica copies the data from the source replica, it will not send the report. Updates kakao#125
ijsong
force-pushed
the
change-report
branch
from
e066495 to
ca4c184
Compare
The performance of metadata repository is not affected. However, it can lead to misjudgment. Let's define a report that can be ignored and use it. |
…port When a log stream replica handles SyncInit, it can be prone to mutate the replica's state too early. It should change the state after checking a valid sync range. It also updates the version, high watermark, and uncommittedLLSNBegin with zeros to denote that the replica sends an invalid report while the learning phase. The replica checks whether it is in the learning phase at the end of method `internal/storagenode/logstream.(*Executor).Report` to minimize the probability of concurrency issues mentioned in kakao#159. Refs - kakao#159 (comment)
What this PR does
Previously, a log stream replica in the learning state sent reports to the metadata repository, and
the log stream replica had to use commit contexts sent from the source replica.
However, the source cannot do that in a future patch because replicas in a log stream will store the
latest commit context rather than all commit contexts.
This patch slightly changes the report's behavior in a log stream replica, not to send it while in a
learning state. When the log stream replica copies the data from the source replica, it will not
send the report.
Which issue(s) this PR resolves
Updates #125
Anything else
RFC: https://github.com/kakao/varlog/blob/main/docs/RFCs/20220915_commit_context.md
This change is