From b90537d562067dc1090456218901cfe3f6383246 Mon Sep 17 00:00:00 2001 From: Phil Pearl Date: Wed, 14 Apr 2021 09:53:01 +0100 Subject: [PATCH 1/5] fix(bigtable/bttest): make table gc release memory We had a problem where we were unable to run the emulator for extended periods in dev environments without giving it huge amounts of RAM, even if we had quite aggressive GC policies applied to the tables. This fixes that by making GC delete empty rows and columns. --- bigtable/bttest/inmem.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/bigtable/bttest/inmem.go b/bigtable/bttest/inmem.go index 56e6d762ba85..71087b8180ba 100644 --- a/bigtable/bttest/inmem.go +++ b/bigtable/bttest/inmem.go @@ -1242,13 +1242,23 @@ func (t *table) gc() { return } + // It isn't clear whether it's safe to delete within the iterator, so we do + // not + var toDelete []btree.Item t.rows.Ascend(func(i btree.Item) bool { r := i.(*row) r.mu.Lock() r.gc(rules) + if len(r.families) == 0 { + toDelete = append(toDelete, i) + } r.mu.Unlock() return true }) + + for _, i := range toDelete { + t.rows.Delete(i) + } } type byRowKey []*row @@ -1334,7 +1344,15 @@ func (r *row) gc(rules map[string]*btapb.GcRule) { continue } for col, cs := range fam.cells { - r.families[fam.name].cells[col] = applyGC(cs, rule) + cs = applyGC(cs, rule) + if len(cs) == 0 { + delete(fam.cells, col) + } else { + fam.cells[col] = cs + } + } + if len(fam.cells) == 0 { + delete(r.families, fam.name) } } } From b383a837c23c18f48e222873a0e948c7d9208655 Mon Sep 17 00:00:00 2001 From: Phil Pearl Date: Wed, 14 Apr 2021 17:23:52 +0100 Subject: [PATCH 2/5] fix(bigtable/bttest) fix locking issue --- bigtable/bttest/inmem.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/bigtable/bttest/inmem.go b/bigtable/bttest/inmem.go index 71087b8180ba..f51f9bd76393 100644 --- a/bigtable/bttest/inmem.go +++ b/bigtable/bttest/inmem.go @@ -1227,6 +1227,19 @@ func (t *table) mutableRow(key string) *row { } func (t *table) gc() { + toDelete := t.gcReadOnly() + if len(toDelete) == 0 { + return + } + + t.mu.Lock() + defer t.mu.Unlock() + for _, i := range toDelete { + t.rows.Delete(i) + } +} + +func (t *table) gcReadOnly() (toDelete []btree.Item) { // This method doesn't add or remove rows, so we only need a read lock for the table. t.mu.RLock() defer t.mu.RUnlock() @@ -1239,12 +1252,11 @@ func (t *table) gc() { } } if len(rules) == 0 { - return + return nil } // It isn't clear whether it's safe to delete within the iterator, so we do // not - var toDelete []btree.Item t.rows.Ascend(func(i btree.Item) bool { r := i.(*row) r.mu.Lock() @@ -1256,9 +1268,7 @@ func (t *table) gc() { return true }) - for _, i := range toDelete { - t.rows.Delete(i) - } + return toDelete } type byRowKey []*row From ff4fa24732fb592dbdca3fd9990dd33cddf79571 Mon Sep 17 00:00:00 2001 From: Phil Pearl Date: Wed, 14 Apr 2021 17:46:32 +0100 Subject: [PATCH 3/5] fix(bigtable/bttest) further race issue in GC --- bigtable/bttest/inmem.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/bigtable/bttest/inmem.go b/bigtable/bttest/inmem.go index f51f9bd76393..90ce1f782cc1 100644 --- a/bigtable/bttest/inmem.go +++ b/bigtable/bttest/inmem.go @@ -1232,10 +1232,18 @@ func (t *table) gc() { return } + // We delete rows that no longer have any cells t.mu.Lock() defer t.mu.Unlock() for _, i := range toDelete { - t.rows.Delete(i) + r := i.(*row) + // Make sure the row still has no cells. We've not been holding a lock + // so it could have changed since we checked it. + r.mu.Lock() + if len(r.families) == 0 { + t.rows.Delete(i) + } + r.mu.Unlock() } } From 74acc2740f4f854a74860e1ad668ef09aed505b2 Mon Sep 17 00:00:00 2001 From: Baha Aiman Date: Fri, 9 Feb 2024 16:12:18 -0800 Subject: [PATCH 4/5] test(bigtable): Adding tests for garbage collection --- bigtable/bttest/inmem_test.go | 67 +++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/bigtable/bttest/inmem_test.go b/bigtable/bttest/inmem_test.go index 3a57585c7e8c..aa03d3c9d0e6 100644 --- a/bigtable/bttest/inmem_test.go +++ b/bigtable/bttest/inmem_test.go @@ -36,6 +36,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/durationpb" ) func TestConcurrentMutationsReadModifyAndGC(t *testing.T) { @@ -467,6 +468,72 @@ func TestModifyColumnFamilies(t *testing.T) { readRows(18, 6, 2) } +func TestGC(t *testing.T) { + // Create server + s := &server{ + tables: make(map[string]*table), + } + ctx := context.Background() + + colFamilyID := "colFam" + colName := "colName" + rowKey := "rowKey" + + // Create table with max age gc rule + newTbl := btapb.Table{ + ColumnFamilies: map[string]*btapb.ColumnFamily{}, + } + newTbl.ColumnFamilies[colFamilyID] = &btapb.ColumnFamily{GcRule: &btapb.GcRule{Rule: &btapb.GcRule_MaxAge{MaxAge: durationpb.New(time.Millisecond)}}} + + tblInfo, err := s.CreateTable(ctx, &btapb.CreateTableRequest{Parent: "cluster", TableId: "t", Table: &newTbl}) + if err != nil { + t.Fatal(err) + } + + // Populate the table + for i := 0; i < 2; i++ { + req := &btpb.MutateRowRequest{ + TableName: tblInfo.Name, + RowKey: []byte(rowKey), + Mutations: []*btpb.Mutation{{ + Mutation: &btpb.Mutation_SetCell_{ + SetCell: &btpb.Mutation_SetCell{ + FamilyName: colFamilyID, + ColumnQualifier: []byte(colName), + TimestampMicros: 1000, // MaxAge is 1ms + Value: []byte{}, + }}, + }}, + } + if _, err := s.MutateRow(ctx, req); err != nil { + t.Fatal(err) + } + } + if err != nil { + t.Fatal(err) + } + + // Sleep till maxAge passes + time.Sleep(2 * time.Millisecond) + + // Trigger gc + tbl := s.tables[tblInfo.Name] + tbl.gc() + + // Verify that the row was deleted after garbage collection + readRowsReq := &btpb.ReadRowsRequest{ + TableName: tblInfo.Name, + Rows: &btpb.RowSet{RowKeys: [][]byte{[]byte(rowKey)}}, + } + mock := &MockReadRowsServer{} + if err = s.ReadRows(readRowsReq, mock); err != nil { + t.Errorf("ReadRows error: %v", err) + } + if got, want := len(mock.responses), 0; got != want { + t.Errorf("response count: got %d, want %d", got, want) + } +} + func TestDropRowRange(t *testing.T) { s := &server{ tables: make(map[string]*table), From 3523e1942c5d4230eacbaf088f28f5b6f0f03617 Mon Sep 17 00:00:00 2001 From: Baha Aiman Date: Thu, 15 Feb 2024 10:35:50 -0800 Subject: [PATCH 5/5] fix(bigtable): Fixing formatting errors --- bigtable/bttest/inmem_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bigtable/bttest/inmem_test.go b/bigtable/bttest/inmem_test.go index 1b3d27571c26..5d9b92497fc3 100644 --- a/bigtable/bttest/inmem_test.go +++ b/bigtable/bttest/inmem_test.go @@ -37,10 +37,10 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/encoding/prototext" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" + "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/wrapperspb" )