colibri: further improve sqlite reservation store

[juagargi]

Improve how segments and end to end reservations are persisted
Document the database design at doc/ColibriService.md

---

This change is 

[oncilla]

Reviewed 22 of 22 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @juagargi)

---

go/cs/reservation/reservationdbtest/reservationdbtest.go, line 668 at r1 (raw file):

			}
		}
		// rsvs = append(rsvs, rs...)

debug remains?

---

go/cs/reservation/sqlite/db.go, line 255 at r1 (raw file):

	deletedIndices := 0

	n, err := deleteExpiredIndices(ctx, x.db, now,

I think keeping it DRY here actually hurts readability.
I think it would be more readable, if you expand the function call here and directly do the calls.

---

go/lib/colibri/reservation/types.go, line 132 at r1 (raw file):

// ToRaw calls Read and returns a new allocated buffer with the ID serialized.
func (id *E2EID) ToRaw() []byte {
	buff := make([]byte, E2EIDLen)

buf

[juagargi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)

---

go/cs/reservation/reservationdbtest/reservationdbtest.go, line 668 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

debug remains?

Done.

---

go/cs/reservation/sqlite/db.go, line 255 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

I think keeping it DRY here actually hurts readability.
I think it would be more readable, if you expand the function call here and directly do the calls.

It's a fairly sized chunk of code to repeat it here. Also DeleteExpiredIndices clearly does twice deleteExpiredIndices. What is not clear IMO is what that function accepts as arguments.
I am adding several comments to improve readability. Please check if it is okay now.

---

go/lib/colibri/reservation/types.go, line 132 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

buf

Done.

[oncilla]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @juagargi)

---

go/cs/reservation/sqlite/db.go, line 255 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

It's a fairly sized chunk of code to repeat it here. Also DeleteExpiredIndices clearly does twice deleteExpiredIndices. What is not clear IMO is what that function accepts as arguments.
I am adding several comments to improve readability. Please check if it is okay now.

it would be something like:

	deletedIndices := 0
	err := db.DoInTx(ctx, x, func(ctx context.Context, tx *sql.Tx) error {
		rowIDs, rsvRowIDs, err := getExpiredIndexRowIDs(ctx, tx, now)
		if err != nil {
			return err
		}
		if len(rowIDs) == 0 {
			// if no index was deleted, nothing else to do
			return nil
		}
		// delete the segment indices pointed by rowIDs
		n, err := deleteE2EIndiciesFromRowIDs(ctx, tx, rowIDs)
		if err != nil {
			return err
		}
		deletedIndices = n
		// delete empty reservations touched by previous removal
		return deleteEmptyE2EReservations(ctx, tx, rsvRowIDs)
	})
	if err != nil {
		return 0, err
	}
	err := db.DoInTx(ctx, x, func(ctx context.Context, tx *sql.Tx) error {
		rowIDs, rsvRowIDs, err := getExpiredSegIndexRowIDs(ctx, tx, now)
		if err != nil {
			return err
		}
		if len(rowIDs) == 0 {
			// if no index was deleted, nothing else to do
			return nil
		}
		// delete the segment indices pointed by rowIDs
		n, err := deleteSegIndiciesFromRowIDs(ctx, tx, rowIDs)
		if err != nil {
			return err
		}
		deletedIndices += n
		// delete empty reservations touched by previous removal
		return deleteEmptySegReservations(ctx, tx, rsvRowIDs)
	})

or you could even do it in a single transaction.

[juagargi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/cs/reservation/sqlite/db.go, line 255 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

it would be something like:

	deletedIndices := 0
	err := db.DoInTx(ctx, x, func(ctx context.Context, tx *sql.Tx) error {
		rowIDs, rsvRowIDs, err := getExpiredIndexRowIDs(ctx, tx, now)
		if err != nil {
			return err
		}
		if len(rowIDs) == 0 {
			// if no index was deleted, nothing else to do
			return nil
		}
		// delete the segment indices pointed by rowIDs
		n, err := deleteE2EIndiciesFromRowIDs(ctx, tx, rowIDs)
		if err != nil {
			return err
		}
		deletedIndices = n
		// delete empty reservations touched by previous removal
		return deleteEmptyE2EReservations(ctx, tx, rsvRowIDs)
	})
	if err != nil {
		return 0, err
	}
	err := db.DoInTx(ctx, x, func(ctx context.Context, tx *sql.Tx) error {
		rowIDs, rsvRowIDs, err := getExpiredSegIndexRowIDs(ctx, tx, now)
		if err != nil {
			return err
		}
		if len(rowIDs) == 0 {
			// if no index was deleted, nothing else to do
			return nil
		}
		// delete the segment indices pointed by rowIDs
		n, err := deleteSegIndiciesFromRowIDs(ctx, tx, rowIDs)
		if err != nil {
			return err
		}
		deletedIndices += n
		// delete empty reservations touched by previous removal
		return deleteEmptySegReservations(ctx, tx, rsvRowIDs)
	})

or you could even do it in a single transaction.

Done.

[oncilla]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

---

go/cs/reservation/sqlite/db.go, line 255 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done.

fwiw, I think this reads nicely 👍

[oncilla]

Reviewable status: complete! all files reviewed, all discussions resolved

[oncilla]

/rebase

[oncilla]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[oncilla]

Reviewable status: complete! all files reviewed, all discussions resolved