Use MaxAssigned for ReadOnly TS #3071
Conversation
We store the max assigned value when processing deltas, we can use that to save an extra network call to zero when getting read-only ts.
The real readTs is MaxTxnTs + 1 not nextTxnTs - 1.
There was a problem hiding this comment.
We need to add another bool to mention "best effort". We shouldn't change the meaning of existing read-only txns. This would require a change at the client side and in Request proto as well.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @srfrog)
worker/mutation.go, line 339 at r1 (raw file):
if num.Val == 0 && num.ReadOnly { if readTs := posting.Oracle().MaxAssigned(); readTs > 0 { glog.V(3).Infof("... Timestamp served from memory: %v", readTs)
Drop the V(3) log.
worker/mutation.go, line 340 at r1 (raw file):
if readTs := posting.Oracle().MaxAssigned(); readTs > 0 { glog.V(3).Infof("... Timestamp served from memory: %v", readTs) return &pb.AssignedIds{ReadOnly: readTs + 1}, nil
This should not be a one above MaxAssigned. It should be at MaxAssigned.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain)
worker/mutation.go, line 339 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Drop the V(3) log.
Done.
worker/mutation.go, line 340 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
This should not be a one above MaxAssigned. It should be at MaxAssigned.
Done.
When the best effort flag is enabled, worker.Timestamps() will try to get the ts from memory.
Add support to the best effort request flag and update the worker.Timestamps() call to use it.
There was a problem hiding this comment.
Line 572 in fde781a
^ This is right place to make a change. If req has best effort (and must also have read only, or we return an error), then here we can get a timestamp from Oracle().MaxAssigned().
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @srfrog)
edgraph/server.go, line 233 at r2 (raw file):
if r.readOnly { num.ReadOnly = true bestEffort = r.bestEffort
we shouldn't be sending a best effort request here.
edgraph/server.go, line 268 at r2 (raw file):
type tsReq struct { readOnly, bestEffort bool
Not required.
worker/mutation.go, line 337 at r2 (raw file):
func Timestamps(ctx context.Context, num *pb.Num, bestEffort bool) (*pb.AssignedIds, error) { if num.Val == 0 && num.ReadOnly && bestEffort {
shouldn't be required to do it here.
There was a problem hiding this comment.
This change as it stands isn't required. Probably better to start from scratch.
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @srfrog)
There was a problem hiding this comment.
Reviewed 2 of 4 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @manishrjain and @srfrog)
edgraph/server.go, line 574 at r3 (raw file):
switch { case req.StartTs == 0:
Not worth a switch case, if it only has one case.
edgraph/server.go, line 582 at r3 (raw file):
} if readTs := posting.Oracle().MaxAssigned(); readTs > 0 { glog.Infof("Timestamp served from memory: %v", readTs)
Remove the infof.
edgraph/server.go, line 587 at r3 (raw file):
} // Our best effort failed, fall back to normal mode. }
This should be else here.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 7 unresolved discussions (waiting on @manishrjain)
edgraph/server.go, line 233 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
we shouldn't be sending a best effort request here.
Done.
edgraph/server.go, line 268 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Not required.
Done.
edgraph/server.go, line 574 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Not worth a switch case, if it only has one case.
Done.
edgraph/server.go, line 582 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Remove the infof.
Done.
edgraph/server.go, line 587 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
This should be else here.
Done.
worker/mutation.go, line 337 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
shouldn't be required to do it here.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)
* worker/mutation.go: use max assigned value from delta in readonly ts We store the max assigned value when processing deltas, we can use that to save an extra network call to zero when getting read-only ts. * worker/draft.go: fixed broken error * worker/mutation.go: ts off by one The real readTs is MaxTxnTs + 1 not nextTxnTs - 1. * edgraph/server.go: add support for best effort request query * worker/mutation.go: read only ts was correct before as maxTxnTs. * worker/mutation.go: expand Timestamps() to support best effort flag When the best effort flag is enabled, worker.Timestamps() will try to get the ts from memory. * edgraph/server.go: add best effort to ts request and enable if needed. Add support to the best effort request flag and update the worker.Timestamps() call to use it. * update for change in worker.Timestamps() func signature, using safe defaults. * edgraph/server.go: revert change to getTimestamp(), use best effort from doQuery() * revert change to worker.Timestamps() * edgraph/server.go: simplify logic a bit * edgraph/server.go: make it simpler (cherry picked from commit 2c765e9)
* worker/mutation.go: use max assigned value from delta in readonly ts We store the max assigned value when processing deltas, we can use that to save an extra network call to zero when getting read-only ts. * worker/draft.go: fixed broken error * worker/mutation.go: ts off by one The real readTs is MaxTxnTs + 1 not nextTxnTs - 1. * edgraph/server.go: add support for best effort request query * worker/mutation.go: read only ts was correct before as maxTxnTs. * worker/mutation.go: expand Timestamps() to support best effort flag When the best effort flag is enabled, worker.Timestamps() will try to get the ts from memory. * edgraph/server.go: add best effort to ts request and enable if needed. Add support to the best effort request flag and update the worker.Timestamps() call to use it. * update for change in worker.Timestamps() func signature, using safe defaults. * edgraph/server.go: revert change to getTimestamp(), use best effort from doQuery() * revert change to worker.Timestamps() * edgraph/server.go: simplify logic a bit * edgraph/server.go: make it simpler (cherry picked from commit 2c765e9)
* worker/mutation.go: use max assigned value from delta in readonly ts We store the max assigned value when processing deltas, we can use that to save an extra network call to zero when getting read-only ts. * worker/draft.go: fixed broken error * worker/mutation.go: ts off by one The real readTs is MaxTxnTs + 1 not nextTxnTs - 1. * edgraph/server.go: add support for best effort request query * worker/mutation.go: read only ts was correct before as maxTxnTs. * worker/mutation.go: expand Timestamps() to support best effort flag When the best effort flag is enabled, worker.Timestamps() will try to get the ts from memory. * edgraph/server.go: add best effort to ts request and enable if needed. Add support to the best effort request flag and update the worker.Timestamps() call to use it. * update for change in worker.Timestamps() func signature, using safe defaults. * edgraph/server.go: revert change to getTimestamp(), use best effort from doQuery() * revert change to worker.Timestamps() * edgraph/server.go: simplify logic a bit * edgraph/server.go: make it simpler
This change will use the stored
MaxAssignedfromProcessDelta()as read-only timestamp. This will save an extra network call to Zero.This change is