Perform incremental rollups instead of rollups at snapshot #4410

parasssh · 2019-12-12T21:35:23Z

This change is

posting/mvcc.go

parasssh

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @golangcibot)

posting/mvcc.go, line 21 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

G505: Blacklisted import crypto/sha1: weak cryptographic primitive (from gosec)

Done.

posting/mvcc.go, line 110 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

G401: Use of weak cryptographic primitive (from gosec)

Done.

posting/mvcc.go, line 113 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 107 characters (from lll)

Done.

posting/mvcc.go, line 115 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of ir.RollUpKey is not checked (from errcheck)

Done.

manishrjain

Bunch of comments.

Reviewable status: 0 of 2 files reviewed, 12 unresolved discussions (waiting on @danielmai, @golangcibot, @manishrjain, @martinmr, and @parasssh)

posting/mvcc.go, line 115 at r1 (raw file):

Previously, parasssh wrote…

Done.

Please check and log it as error, so the user knows.

posting/mvcc.go, line 39 at r2 (raw file):

// IncRollup is used to batch keys for rollup incrementally.
type IncRollup struct {

Does this need to be in posting, or can it lie in worker package?

posting/mvcc.go, line 41 at r2 (raw file):

type IncRollup struct {
	// Ch is populated with batch of 64 keys that needs to be rolled up during reads
	Ch chan *[][]byte

does it need to be public? Also, Ch for what? Maybe call it keysCh.

posting/mvcc.go, line 43 at r2 (raw file):

	Ch chan *[][]byte
	// Pool is sync.Pool to share the batched keys to rollup.
	Pool sync.Pool

don't need to be public, right? Also, Pool isn't a great name. Maybe call it keysPool.

posting/mvcc.go, line 72 at r2 (raw file):

	}

	err = writer.Write(&bpb.KVList{Kv: kvs})

return writer.Write(...)

posting/mvcc.go, line 92 at r2 (raw file):

	default:
		// XXX/pshah: Instead of dropping keys and starting to build the batch again,
		// maybe we should try again later  -- check with mrjn

No need to try again. This should be lossy to avoid contention. You can mention that here.

posting/mvcc.go, line 100 at r2 (raw file):

// HandleIncrementalRollups will rollup batches of 64 keys in a go routine.
func (ir *IncRollup) HandleIncrementalRollups() {
	m := make(map[uint64]int64) // map from hash(key) to timestamp

Can the ts be in int32?

posting/mvcc.go, line 101 at r2 (raw file):

func (ir *IncRollup) HandleIncrementalRollups() {
	m := make(map[uint64]int64) // map from hash(key) to timestamp
	limiter := time.Tick(10 * time.Millisecond)

If you're doing 64 of them per limit, then make this 100 ms.

posting/mvcc.go, line 108 at r2 (raw file):

		for _, key := range *batch {
			hashBytes := sha256.Sum256(key)
			hash := binary.BigEndian.Uint64(hashBytes[0:]) // 1st 8 bytes of the SHA256 hash

Use farm.Fingerprint instead. Or, even better use the z.MemHash (from ristretto).

parasssh

Dismissed @golangcibot from 4 discussions.
Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)

posting/mvcc.go, line 39 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Does this need to be in posting, or can it lie in worker package?

There was a circular dependency in putting in worker pkg.

posting/mvcc.go, line 41 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

does it need to be public? Also, Ch for what? Maybe call it keysCh.

Done.

posting/mvcc.go, line 43 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

don't need to be public, right? Also, Pool isn't a great name. Maybe call it keysPool.

Done.

posting/mvcc.go, line 72 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

return writer.Write(...)

Done.

posting/mvcc.go, line 92 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need to try again. This should be lossy to avoid contention. You can mention that here.

Done.

posting/mvcc.go, line 100 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Can the ts be in int32?

z.MemHash and time.Unix returns a 64 bit integer. So, using that.

posting/mvcc.go, line 101 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

If you're doing 64 of them per limit, then make this 100 ms.

Done.

posting/mvcc.go, line 108 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Use farm.Fingerprint instead. Or, even better use the z.MemHash (from ristretto).

Done.

parasssh

Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)

posting/mvcc.go, line 39 at r2 (raw file):

Previously, parasssh wrote…

There was a circular dependency in putting in worker pkg.

Done.

parasssh

Dismissed @manishrjain from 8 discussions.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @danielmai and @martinmr)

martinmr

Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @danielmai and @parasssh)

posting/mvcc.go, line 51 at r3 (raw file):

IncrRollup = IncRollup{make(chan *[][]byte),

minor: new line before make for readibility.

posting/mvcc.go, line 94 at r3 (raw file):

// HandleIncrementalRollups will rollup batches of 64 keys in a go routine.
func (ir *IncRollup) HandleIncrementalRollups() {
	m := make(map[uint64]int64) // map from hash(key) to timestamp

Maybe add a comment regarding why the keys are being hashed. I am assuming it's to prevent the map from growing too large but it's unclear rn. I think a comment would help.

posting/mvcc.go, line 95 at r3 (raw file):

func (ir *IncRollup) HandleIncrementalRollups() {
	m := make(map[uint64]int64) // map from hash(key) to timestamp
	limiter := time.Tick(100 * time.Millisecond)

There's a DeepSource warning about this line leaking resources. Please look into this.

https://deepsource.io/gh/dgraph-io/dgraph/run/3621b864-1dd7-44e3-956e-03a3fb76ec19/#go

posting/mvcc.go, line 291 at r3 (raw file):

if deltaCount >= 2 {

define the 2 as a constant (e.g maxDeltaCount or similar).

worker/draft.go, line 1010 at r3 (raw file):

// list, and write back a complete posting list.
func (n *node) rollupLists(readTs uint64) error {
	//writer := posting.NewTxnWriter(pstore)

commented code.

worker/draft.go, line 1039 at r3 (raw file):

	}

	stream := pstore.NewStreamAt(readTs)

Do we still need this stream? If I am understanding correctly, it's only being kept for calculating the list size right.

If that's the case, I would change the LogPrefix to something clearer since this stream is not rolling up stuff anymore.

parasssh

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @danielmai and @martinmr)

posting/mvcc.go, line 94 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Maybe add a comment regarding why the keys are being hashed. I am assuming it's to prevent the map from growing too large but it's unclear rn. I think a comment would help.

Done.

posting/mvcc.go, line 95 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

There's a DeepSource warning about this line leaking resources. Please look into this.

https://deepsource.io/gh/dgraph-io/dgraph/run/3621b864-1dd7-44e3-956e-03a3fb76ec19/#go

done.

posting/mvcc.go, line 291 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

if deltaCount >= 2 {
define the 2 as a constant (e.g maxDeltaCount or similar).

Done.

worker/draft.go, line 1010 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

commented code.

Done.

worker/draft.go, line 1039 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Do we still need this stream? If I am understanding correctly, it's only being kept for calculating the list size right.

If that's the case, I would change the LogPrefix to something clearer since this stream is not rolling up stuff anymore.

Done.

parasssh

Dismissed @martinmr from 5 discussions.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @danielmai and @martinmr)

golangcibot · 2020-01-03T22:33:15Z

posting/mvcc.go

+
+// HandleIncrementalRollups will rollup batches of 64 keys in a go routine.
+func (ir *IncRollup) HandleIncrementalRollups() {
+	m := make(map[uint64]int64) // map from hash(key) to timestamp. hash(key) to limit the size of the map.


line is 104 characters (from lll)

parasssh

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai, @golangcibot, and @martinmr)

posting/mvcc.go, line 95 at r4 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 104 characters (from lll)

Done.

parasssh

Dismissed @golangcibot from a discussion.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @danielmai and @martinmr)

manishrjain

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @danielmai, @martinmr, and @parasssh)

posting/mvcc.go, line 39 at r5 (raw file):

// IncRollup is used to batch keys for rollup incrementally.
type IncRollup struct {

IncrRollupi

posting/mvcc.go, line 52 at r5 (raw file):

	// IncrRollup is used to batch keys for rollup incrementally.
	IncrRollup = IncRollup{
		make(chan *[][]byte),

keysCh: ...

posting/mvcc.go, line 53 at r5 (raw file):

	IncrRollup = IncRollup{
		make(chan *[][]byte),
		sync.Pool{

keysPool: ...

worker/draft.go, line 1047 at r5 (raw file):

			return false
		default:
			return false

Do add a comment why everything is false.

worker/draft.go, line 1430 at r5 (raw file):

	go n.processApplyCh()
	go n.BatchAndSendMessages()
	go posting.IncrRollup.HandleIncrementalRollups()

go posting.IncrRollup.Process()

parasssh

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @danielmai, @manishrjain, @martinmr, and @parasssh)

posting/mvcc.go, line 39 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

IncrRollupi

Done.

posting/mvcc.go, line 53 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

keysPool: ...

Done.

worker/draft.go, line 1047 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Do add a comment why everything is false.

Done.

worker/draft.go, line 1430 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

go posting.IncrRollup.Process()

Done.

parasssh

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)

posting/mvcc.go, line 52 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

keysCh: ...

Done.

parasssh

Dismissed @manishrjain from 5 discussions.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @danielmai and @martinmr)

mangalaman93

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @danielmai, @martinmr, and @parasssh)

posting/mvcc.go, line 41 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

does it need to be public? Also, Ch for what? Maybe call it keysCh.

And why is this need to be a pointer? Slice are passed as a reference.

posting/mvcc.go, line 43 at r2 (raw file):

	Ch chan *[][]byte
	// Pool is sync.Pool to share the batched keys to rollup.
	Pool sync.Pool

Pool is exported too, required?

posting/mvcc.go, line 54 at r2 (raw file):

		sync.Pool{
			New: func() interface{} {
				return new([][]byte)

Two questions?

Why use sync pool at all? For concurrent access? We are only create slices of slice which would consume 64*16 bytes which is not a lot of memory per entry in the pool.
Why not create a slice of capacity 64 here? We know our slices are not going to grow beyond that.

posting/mvcc.go, line 72 at r2 (raw file):

Previously, parasssh wrote…

Done.

Not done!

posting/mvcc.go, line 91 at r2 (raw file):

	case ir.Ch <- batch:
	default:
		// XXX/pshah: Instead of dropping keys and starting to build the batch again,

Why can't we just do a blocking wait? We should at least do a timeout before we decide to clear the batch.

posting/mvcc.go, line 107 at r2 (raw file):

		currTs := time.Now().Unix()
		for _, key := range *batch {
			hashBytes := sha256.Sum256(key)

Why not use z.MemHash here?

posting/mvcc.go, line 112 at r2 (raw file):

				// Key not present or Key present but last roll up was more than 10 sec ago.
				// Add/Update map and rollup.
				m[hash] = currTs

Wouldn't there be too many collisions after a point?

posting/mvcc.go, line 113 at r2 (raw file):

				// Add/Update map and rollup.
				m[hash] = currTs
				_ = ir.RollUpKey(writer, key)

We should log the error unless errors are expected to be often

parasssh

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @danielmai, @mangalaman93, and @martinmr)

posting/mvcc.go, line 41 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

And why is this need to be a pointer? Slice are passed as a reference.

it is so that we only pass a pointer value in the channel.

posting/mvcc.go, line 43 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Pool is exported too, required?

No, it isnt.

posting/mvcc.go, line 54 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Two questions?

Why use sync pool at all? For concurrent access? We are only create slices of slice which would consume 64*16 bytes which is not a lot of memory per entry in the pool.

Why not create a slice of capacity 64 here? We know our slices are not going to grow beyond that.

This was Manish's suggestion. I believe the idea was to eventually allow for more keys to be batched. And further, allocating/de-allocating such slices may be wasteful. The Sync.Pool allows reuse of these slices.

posting/mvcc.go, line 72 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Not done!

Yes, it is done. Are you looking at an older revision?

posting/mvcc.go, line 91 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Why can't we just do a blocking wait? We should at least do a timeout before we decide to clear the batch.

this code is in the critical path. It is called in ReadPostingList and hence blocking is not an option. There isnt a timeout since we are allowing a Lossy behavior per Manish's suggestion.

posting/mvcc.go, line 107 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Why not use z.MemHash here?

It is. Are you looking at the correct revision?

posting/mvcc.go, line 112 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Wouldn't there be too many collisions after a point?

Are you saying map's collisions or the key collisions itself since it is a hash. If latter, then I am not concerned since we have a 64bit space. If former, then yes eventually it may happen but I believe go-lang's map implementation has a collision-resistant hash function underneath and efficient.

posting/mvcc.go, line 113 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

We should log the error unless errors are expected to be often

err is logged in the next line. Are you seeing the latest revision?

parasssh

Dismissed @mangalaman93 from 6 discussions.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @danielmai and @martinmr)

manishrjain

Got a bunch of comments. Please carefully address them and then merge.

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai and @parasssh)

posting/mvcc.go, line 41 at r2 (raw file):

Previously, parasssh wrote…

it is so that we only pass a pointer value in the channel.

Slices are already pointers.

posting/mvcc.go, line 43 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Pool is exported too, required?

Actually, shouldn't this be a pointer?

posting/mvcc.go, line 39 at r5 (raw file):

Previously, parasssh wrote…

Done.

Add the r please in Incr. And make this private: incrRollupi

posting/mvcc.go, line 51 at r6 (raw file):

	// IncrRollup is used to batch keys for rollup incrementally.
	IncrRollup = IncRollupi{

I'd make this a pointer

IncrRollup = &incrRollupi

posting/mvcc.go, line 107 at r6 (raw file):

				// Add/Update map and rollup.
				m[hash] = currTs
				err := ir.rollUpKey(writer, key)

if err := ... ; err != nil

worker/draft.go, line 1047 at r5 (raw file):

Previously, parasssh wrote…

Done.

I don't see the comment.

worker/draft.go, line 1019 at r6 (raw file):

	// We're doing rollups. We should use this opportunity to calculate the tablet sizes.
	amLeader := n.AmLeader()
	if !amLeader {

Is this variable used anywhere else? If not, then remove the variable.

parasssh

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai and @manishrjain)

posting/mvcc.go, line 51 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd make this a pointer

IncrRollup = &incrRollupi

Done.

posting/mvcc.go, line 107 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if err := ... ; err != nil

Done.

worker/draft.go, line 1047 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I don't see the comment.

Done.

worker/draft.go, line 1019 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Is this variable used anywhere else? If not, then remove the variable.

Done. Removed the variable.

parasssh

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @danielmai and @manishrjain)

posting/mvcc.go, line 41 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Slices are already pointers.

So, it seems DeepSource is complaining. It says
"When passing a value that is not a pointer to a function that accepts an interface, the value needs to be placed on the heap, which means an additional allocation. Slices are a common thing to put in sync.Pools, and they’re structs with 3 fields (length, capacity, and a pointer to an array). In order to avoid the extra allocation, one should store a pointer to the slice instead. "

I guess that is why I had this as a pointer to slice in the first place.

mangalaman93

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

posting/mvcc.go, line 91 at r2 (raw file):

Previously, parasssh wrote…

this code is in the critical path. It is called in ReadPostingList and hence blocking is not an option. There isnt a timeout since we are allowing a Lossy behavior per Manish's suggestion.

One more question, why do we decide to drop the batch? We can just put the batch back into the pool.
We should at least put the empty slice back into the pool in case we decide to drop the batch.

posting/mvcc.go, line 112 at r2 (raw file):

Previously, parasssh wrote…

Are you saying map's collisions or the key collisions itself since it is a hash. If latter, then I am not concerned since we have a 64bit space. If former, then yes eventually it may happen but I believe go-lang's map implementation has a collision-resistant hash function underneath and efficient.

I think the older code was using 8 bit hash. This part makes sense. Though, this map will always keep growing forever, could cause memory problems.

posting/mvcc.go, line 97 at r7 (raw file):

	m := make(map[uint64]int64) // map hash(key) to ts. hash(key) to limit the size of the map.
	limiter := time.NewTicker(100 * time.Millisecond)
	writer := NewTxnWriter(pstore)

We never call writer.Flush(). Not sure whether that could cause problems. I wonder whether using normal transaction would be okay too here given that this is not in the critical path.

worker/draft.go, line 1013 at r7 (raw file):

// rollupLists would consolidate all the deltas that constitute one posting
// list, and write back a complete posting list.
func (n *node) rollupLists(readTs uint64) error {

This function doesn't do rollups any more. We can rename it to what it is doing now.

parasssh

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @danielmai, @mangalaman93, and @manishrjain)

posting/mvcc.go, line 91 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

One more question, why do we decide to drop the batch? We can just put the batch back into the pool.
We should at least put the empty slice back into the pool in case we decide to drop the batch.

I had it that way earlier but Manish argued to drop it / make it lossy. Also, the empty slice is indeed Put into the pool (see next couple of lines)

posting/mvcc.go, line 97 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

We never call writer.Flush(). Not sure whether that could cause problems. I wonder whether using normal transaction would be okay too here given that this is not in the critical path.

I remember Manish saying its not needed. Also, I did the 1M and 21M dataset and used debug tool to spot check.

worker/draft.go, line 1013 at r7 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This function doesn't do rollups any more. We can rename it to what it is doing now.

Done.

parasssh

Dismissed @mangalaman93 and @manishrjain from 5 discussions.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @danielmai and @manishrjain)

…4410)" This reverts commit 42b2a60. Incremental rollups PR is causing Jepsen failures. Reverting this PR for now, until we figure out a fix for it.

…pshot (#4410)"" This reverts commit 57474e0.

Paras Shah added 6 commits November 21, 2019 10:22

best-effort / lossy on sender; batch on receiver; use sync.pool

31d62e4

cleanups, map uses hash(key), struct, bring all methods in struct type

91343eb

go fmt

c68273c

Merge remote-tracking branch 'origin/master' into ps_incremental_rollups

d5eb7a4

go fmt

109d077

import fix

4f0b6ab

golangcibot reviewed Dec 12, 2019

View reviewed changes

posting/mvcc.go Outdated Show resolved Hide resolved

posting/mvcc.go Outdated Show resolved Hide resolved

posting/mvcc.go Outdated Show resolved Hide resolved

posting/mvcc.go Outdated Show resolved Hide resolved

Paras Shah added 2 commits December 12, 2019 15:29

bot comments

ecda6fb

cleanup

3d68c4f

parasssh commented Dec 12, 2019

View reviewed changes

parasssh requested review from manishrjain and danielmai December 13, 2019 05:56

martinmr marked this pull request as ready for review December 13, 2019 18:50

martinmr requested review from martinmr and a team as code owners December 13, 2019 18:50

manishrjain suggested changes Dec 17, 2019

View reviewed changes

parasssh commented Jan 3, 2020

View reviewed changes

address Manish's review comments

1f23de2

parasssh commented Jan 3, 2020

View reviewed changes

dont make rollupKey public

816c539

martinmr suggested changes Jan 3, 2020

View reviewed changes

address review comments

0c33fc5

parasssh commented Jan 3, 2020

View reviewed changes

golangcibot reviewed Jan 3, 2020

View reviewed changes

golang ci fix

3a01589

parasssh commented Jan 6, 2020

View reviewed changes

manishrjain suggested changes Jan 6, 2020

View reviewed changes

parasssh commented Jan 7, 2020

View reviewed changes

address review comments

7ae4b36

parasssh commented Jan 7, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into ps_incremental_rollups

bfef6e7

mangalaman93 requested a review from martinmr January 8, 2020 07:12

mangalaman93 requested changes Jan 8, 2020

View reviewed changes

parasssh commented Jan 8, 2020

View reviewed changes

manishrjain approved these changes Jan 8, 2020

View reviewed changes

parasssh commented Jan 9, 2020

View reviewed changes

address review comments

0c8b082

parasssh commented Jan 9, 2020

View reviewed changes

revert back to using pointer to slice as DeepSource complained

ddfe38a

mangalaman93 requested a review from manishrjain January 9, 2020 14:01

mangalaman93 requested changes Jan 9, 2020

View reviewed changes

addr review comments; minor cleanups

c9aa143

parasssh commented Jan 9, 2020

View reviewed changes

parasssh merged commit 42b2a60 into master Jan 9, 2020

parasssh pushed a commit that referenced this pull request Jan 23, 2020

Revert "Revert "Perform incremental rollups instead of rollups at sna…

fc94a00

…pshot (#4410)"" This reverts commit 57474e0.

parasssh mentioned this pull request Jan 23, 2020

Re-introduce Incremental Rollup patch. #4647

Closed

parasssh pushed a commit that referenced this pull request Jan 28, 2020

Revert "Revert "Perform incremental rollups instead of rollups at sna…

bf0f6d0

…pshot (#4410)"" This reverts commit 57474e0.

parasssh deleted the ps_incremental_rollups branch January 30, 2020 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform incremental rollups instead of rollups at snapshot #4410

Perform incremental rollups instead of rollups at snapshot #4410

parasssh commented Dec 12, 2019 •

edited by manishrjain

Loading

parasssh left a comment

manishrjain left a comment

parasssh left a comment

parasssh left a comment

parasssh left a comment

martinmr left a comment

parasssh left a comment

parasssh left a comment

golangcibot Jan 3, 2020

parasssh left a comment

parasssh left a comment

manishrjain left a comment

parasssh left a comment

parasssh left a comment

parasssh left a comment

mangalaman93 left a comment

parasssh left a comment

parasssh left a comment

manishrjain left a comment

parasssh left a comment

parasssh left a comment

mangalaman93 left a comment

parasssh left a comment

parasssh left a comment

Perform incremental rollups instead of rollups at snapshot #4410

Perform incremental rollups instead of rollups at snapshot #4410

Conversation

parasssh commented Dec 12, 2019 • edited by manishrjain Loading

parasssh left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

golangcibot Jan 3, 2020

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh commented Dec 12, 2019 •

edited by manishrjain

Loading