Encapsulation of applier logic: Move Txn related code out of applier.go. #13878
Conversation
f68269b
to
5bb0497
Compare
Codecov Report
@@ Coverage Diff @@
## main #13878 +/- ##
==========================================
- Coverage 72.64% 72.36% -0.29%
==========================================
Files 469 474 +5
Lines 38413 38464 +51
==========================================
- Hits 27906 27834 -72
- Misses 8733 8841 +108
- Partials 1774 1789 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
server/etcdserver/server.go
Outdated
|
|
||
| // As backends and implementations like alarmsStore changed, we need | ||
| // to re-bootstrap Appliers. | ||
| s.uberApply = newUberApplier(s) |
There was a problem hiding this comment.
Instead of storing a alarmStore reference, could we store reference to alarmStore getter interface?
I'm worried of pattern if Server.X field changes we need to regenerate Server.Y field
There was a problem hiding this comment.
I would agree in general, but it would require deep reform what Backend is and how restore from snapshot works.
Currently restore from snapshot:
- creates a new instance of the backend
- all the objects/helpers pointing on the all backend are 'poinsoned'.
- we need to rebootstrap them using the new backend
It might be easier / safer to create them from scratch rather make sure there is foo.RebootstrapFromBackend(BE) method that will entirely clean it's state and will load what needed from be
There was a problem hiding this comment.
I think we can revisit this comment in a separate PR if needed.
8b711a8
to
0d20e75
Compare
|
0d20e75
to
90d859e
Compare
|
Is this PR planned on 3.6 or 3.7? I assume it's 3.7, otherwise it will be risky to cherry pick to 3.5. Are you going to move all read-only transactions out of the normal raft consensus protocol (apply workflow), and instead depends on linearizableReadLoop in the next step? In this case, the consistent_index will only be moved forward by the mvcc transactions? It seems that there are still some big refactors to completely resolve issues/13766. |
I assumed 3.6 - it's early in the release train I think.
No. These transactions (RO through apply) are not harmful as they are fully idempotent. The idea was to split the serving stack into Currently: Splitted stack serving 'applies' and read-only calls: Practically needed stack due to performance reasons: But we need to be careful that any 'async' calls will not influence cache content such that it can meaningful impact on the 'apply' determinism. But all of this seems indeed complex for 3.6. So in short-term I'm thinking about: That makes a sandwich around existing logic, with verification of origin of the transaction. |
If it's planned for 3.6, then my PR 13854 is definitely not needed any more, at least not needed for 3.6. What's the plan for 3.5?
I do not get time to review through this PR (will probably had a deep review tomorrow), but based on my quick review this morning, it seems that this PR has already completed this short-term, correct? |
|
@ahrtr PTAL #13854 (comment) |
c3811d2
to
ad920ed
Compare
ad920ed
to
8ff3ff7
Compare
Putting on side future directions, do you think that this refactoring is not worthwhile on its own ?
|
|
You're right, it's not an alternative, but supplementary improvement. We would greatly benefit from it. |
cb80a48
to
7fe3ab2
Compare
| } | ||
| if corruptAlarms { | ||
| a.applyV3 = newApplierV3Corrupt(a.applyV3) | ||
| } |
There was a problem hiding this comment.
Can both noSpaceAlarms and corruptAlarms be true? If not, then please add a comment to clarify or update the code to avoid confusion.
There was a problem hiding this comment.
Unlikely but theoretically possible. This preserves the original logic:
etcd/server/etcdserver/server.go
Lines 2274 to 2279 in 52ea811
There was a problem hiding this comment.
minor comment: Probably we should add a verification (only one kind of alarm can exist) here in case whatever potential bug or error.
There was a problem hiding this comment.
I don't see why only 1 alarm is an invariant of the system.
What if we are in NOSPACE alarm and the corruption check will declare the in-consistency ?
I assume the applier will have 2 wrappers, but everything should work as expected.
There was a problem hiding this comment.
You do not change it in this PR, Let's keep it as it is for now. We can address it separately if needed.
7fe3ab2
to
5bd8383
Compare
|
This is a huge PR, which in general I don't think it's good for review. It took me huge time to review this PR, although I still don't think it's well reviewed, but overall it looks good to me. If I do this PR, I will breakdown it to three PRs at least. The first PR just moves all apply related code into package/folder In summary, I'd like to let this PR to go, reasons:
|
|
Thank you and really appreciate your time and insightful review.
I tried to make it commit by commit. Just move of 'apply' is infeasible because of spaghettini code. Probably |
server/etcdserver/server.go
Outdated
| } | ||
|
|
||
| func (s *EtcdServer) NewUberApplier() *uberApplier { | ||
| return newUberApplier(s.lg, s.be, s.KV(), s.alarmStore, s.authStore, s.lessor, s.cluster, s, s, s.consistIndex, |
There was a problem hiding this comment.
Are we sure that during runtime (after bootstrap) none of those fields on server are recreated? For example restoring snapshot, recovering auth etcd?
There was a problem hiding this comment.
They are in the applySnapshot and this is handled. I haven't found other places were we 'recreate' them.
The PR removes calls to applierV3base logic from server.go that is NOT part of 'application'. The original idea was that read-only transaction and Range call shared logic with Apply, so they can call appliers directly (but bypassing all 'corrupt', 'quota' and 'auth' wrappers). This PR moves all the logic to a separate file (that later can become package on its own).
… server
This PR:
- moves wrapping of appliers (due to Alarms) out of server.go into uber_applier.go
- clearly devides the application logic into: chain of:
a) 'WrapApply' (generic logic across all the methods)
b) dispatcher (translation of Apply into specific method like 'Put')
c) chain of 'wrappers' of the specific methods (like Put).
- when we do recovery (restore from snapshot) we create new instance of appliers.
The purpose is to make sure we control all the depencies of the apply process, i.e.
we can supply e.g. special instance of 'backend' to the application logic.
It was misleading and error prone vs. ClusterId.
All the depencies are explicily passed to the UberApplier factory method.
Side effect: applySec(0.4s) used to be reported as 0s, now it's correctly 0.4s.
f28bfa4
to
41ff237
Compare
|
FTR: Flake due to port conflict: |
|
LGTM |
| if !ok || tv.RequestTxn == nil { | ||
| continue | ||
| } | ||
| txnPath = append(txnPath, compareToPath(rv, tv.RequestTxn)...) |
There was a problem hiding this comment.
What if the nested transaction's conditions become different after applying previous ops?
There was a problem hiding this comment.
Yeah. That's how the current logic is implemented. But it seems not quite intuitive. Said in transaction, users can assume the requests are handled one by one, so the following requests should be able to see previous mutations.
etcd implements read after write, that if you write put k1 v1 and get k1 in one transaction, get k1 sees v1, but it's not applied for compares.
The PR removes calls to applierV3base logic from server.go that is NOT part of 'application'.
The original idea was that read-only transaction and Range call shared logic with Apply,
so they can call appliers directly (but bypassing all 'corrupt', 'quota' and 'auth' wrappers).
This PR moves all the logic to a separate file (that later can become package on its own).
This is preparation to fix: #13766 by wrapping backends.