From d8885c5f9b7d9d252d8cd8d95e4f5d21afefd3ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Barbosa=20Sampaio?= Date: Mon, 5 Feb 2024 15:43:01 -0500 Subject: [PATCH 1/5] feat(bigtable): Add ignore_warnings flag to SetGcPolicy --- bigtable/admin.go | 11 ++++++++++- bigtable/admin_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/bigtable/admin.go b/bigtable/admin.go index b988b7075090..120e5a669c86 100644 --- a/bigtable/admin.go +++ b/bigtable/admin.go @@ -490,7 +490,7 @@ func (ac *AdminClient) TableInfo(ctx context.Context, table string) (*TableInfo, // SetGCPolicy specifies which cells in a column family should be garbage collected. // GC executes opportunistically in the background; table reads may return data // matching the GC policy. -func (ac *AdminClient) SetGCPolicy(ctx context.Context, table, family string, policy GCPolicy) error { +func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, ignoreWarnings bool) error { ctx = mergeOutgoingMetadata(ctx, ac.md) prefix := ac.instancePrefix() req := &btapb.ModifyColumnFamiliesRequest{ @@ -499,11 +499,20 @@ func (ac *AdminClient) SetGCPolicy(ctx context.Context, table, family string, po Id: family, Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{Update: &btapb.ColumnFamily{GcRule: policy.proto()}}, }}, + IgnoreWarnings: ignoreWarnings, } _, err := ac.tClient.ModifyColumnFamilies(ctx, req) return err } +func (ac *AdminClient) SetGCPolicyIgnoreWarnings(ctx context.Context, table, family string, policy GCPolicy) error { + return ac.setGCPolicy(ctx, table, family, policy, true) +} + +func (ac *AdminClient) SetGCPolicy(ctx context.Context, table, family string, policy GCPolicy) error { + return ac.setGCPolicy(ctx, table, family, policy, false) +} + // DropRowRange permanently deletes a row range from the specified table. func (ac *AdminClient) DropRowRange(ctx context.Context, table, rowKeyPrefix string) error { ctx = mergeOutgoingMetadata(ctx, ac.md) diff --git a/bigtable/admin_test.go b/bigtable/admin_test.go index 95f864029514..ede1900c2cff 100644 --- a/bigtable/admin_test.go +++ b/bigtable/admin_test.go @@ -41,6 +41,8 @@ type mockTableAdminClock struct { copyBackupReq *btapb.CopyBackupRequest copyBackupError error + + modColumnReq *btapb.ModifyColumnFamiliesRequest } func (c *mockTableAdminClock) CreateTable( @@ -70,6 +72,12 @@ func (c *mockTableAdminClock) CopyBackup( return nil, c.copyBackupError } +func (c *mockTableAdminClock) ModifyColumnFamilies( + ctx context.Context, in *btapb.ModifyColumnFamiliesRequest, opts ...grpc.CallOption) (*btapb.Table, error) { + c.modColumnReq = in + return nil, nil +} + func setupTableClient(t *testing.T, ac btapb.BigtableTableAdminClient) *AdminClient { ctx := context.Background() c, err := NewAdminClient(ctx, "my-cool-project", "my-cool-instance") @@ -277,6 +285,36 @@ func TestTableAdmin_UpdateTableDisableChangeStream(t *testing.T) { } } +func TestTableAdmin_SetGcPolicy(t *testing.T) { + mock := &mockTableAdminClock{} + c := setupTableClient(t, mock) + + err := c.SetGCPolicy(context.Background(), "My-table", "cf1", NoGcPolicy()) + if err != nil { + t.Fatalf("Failed to set GC Policy: %v", err) + } + + modColumnReq := mock.modColumnReq + if modColumnReq.IgnoreWarnings { + t.Errorf("IgnoreWarnings should be set to false") + } +} + +func TestTableAdmin_SetGcPolicyIgnoreWarnings(t *testing.T) { + mock := &mockTableAdminClock{} + c := setupTableClient(t, mock) + + err := c.SetGCPolicyIgnoreWarnings(context.Background(), "My-table", "cf1", NoGcPolicy()) + if err != nil { + t.Fatalf("Failed to set GC Policy: %v", err) + } + + modColumnReq := mock.modColumnReq + if !modColumnReq.IgnoreWarnings { + t.Errorf("IgnoreWarnings should be set to true") + } +} + type mockAdminClock struct { btapb.BigtableInstanceAdminClient From 56f7198cd0262e9abdfc6b0653ac87c8a1157e50 Mon Sep 17 00:00:00 2001 From: Baha Aiman Date: Tue, 16 Apr 2024 03:49:53 +0000 Subject: [PATCH 2/5] feat(bigtable): Add GcPolicyOptions --- bigtable/admin.go | 14 +++++++----- bigtable/admin_test.go | 48 ++++++++++++++++++++++++++++-------------- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/bigtable/admin.go b/bigtable/admin.go index 172c696908af..1d85d84880eb 100644 --- a/bigtable/admin.go +++ b/bigtable/admin.go @@ -546,7 +546,7 @@ func (ac *AdminClient) TableInfo(ctx context.Context, table string) (*TableInfo, // SetGCPolicy specifies which cells in a column family should be garbage collected. // GC executes opportunistically in the background; table reads may return data // matching the GC policy. -func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, ignoreWarnings bool) error { +func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, opts GCPolicyOptions) error { ctx = mergeOutgoingMetadata(ctx, ac.md) prefix := ac.instancePrefix() req := &btapb.ModifyColumnFamiliesRequest{ @@ -555,18 +555,22 @@ func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, po Id: family, Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{Update: &btapb.ColumnFamily{GcRule: policy.proto()}}, }}, - IgnoreWarnings: ignoreWarnings, + IgnoreWarnings: opts.IgnoreWarnings, } _, err := ac.tClient.ModifyColumnFamilies(ctx, req) return err } -func (ac *AdminClient) SetGCPolicyIgnoreWarnings(ctx context.Context, table, family string, policy GCPolicy) error { - return ac.setGCPolicy(ctx, table, family, policy, true) +type GCPolicyOptions struct { + IgnoreWarnings bool +} + +func (ac *AdminClient) SetGCPolicyWithOptions(ctx context.Context, table, family string, policy GCPolicy, opts GCPolicyOptions) error { + return ac.setGCPolicy(ctx, table, family, policy, opts) } func (ac *AdminClient) SetGCPolicy(ctx context.Context, table, family string, policy GCPolicy) error { - return ac.setGCPolicy(ctx, table, family, policy, false) + return ac.setGCPolicy(ctx, table, family, policy, GCPolicyOptions{}) } // DropRowRange permanently deletes a row range from the specified table. diff --git a/bigtable/admin_test.go b/bigtable/admin_test.go index ede1900c2cff..8d9d3ad616fa 100644 --- a/bigtable/admin_test.go +++ b/bigtable/admin_test.go @@ -42,7 +42,7 @@ type mockTableAdminClock struct { copyBackupReq *btapb.CopyBackupRequest copyBackupError error - modColumnReq *btapb.ModifyColumnFamiliesRequest + modColumnReq *btapb.ModifyColumnFamiliesRequest } func (c *mockTableAdminClock) CreateTable( @@ -286,32 +286,48 @@ func TestTableAdmin_UpdateTableDisableChangeStream(t *testing.T) { } func TestTableAdmin_SetGcPolicy(t *testing.T) { - mock := &mockTableAdminClock{} - c := setupTableClient(t, mock) + for _, test := range []struct { + desc string + opts GCPolicyOptions + want bool + }{ + { + desc: "IgnoreWarnings: false", + opts: GCPolicyOptions{}, + want: false, + }, + { + desc: "IgnoreWarnings: true", + opts: GCPolicyOptions{IgnoreWarnings: true}, + want: true, + }, + } { - err := c.SetGCPolicy(context.Background(), "My-table", "cf1", NoGcPolicy()) - if err != nil { - t.Fatalf("Failed to set GC Policy: %v", err) - } + mock := &mockTableAdminClock{} + c := setupTableClient(t, mock) - modColumnReq := mock.modColumnReq - if modColumnReq.IgnoreWarnings { - t.Errorf("IgnoreWarnings should be set to false") + err := c.SetGCPolicyWithOptions(context.Background(), "My-table", "cf1", NoGcPolicy(), test.opts) + if err != nil { + t.Fatalf("%v: Failed to set GC Policy: %v", test.desc, err) + } + + modColumnReq := mock.modColumnReq + if modColumnReq.IgnoreWarnings != test.want { + t.Errorf("%v: IgnoreWarnings got: %v, want: %v", test.desc, modColumnReq.IgnoreWarnings, test.want) + } } -} -func TestTableAdmin_SetGcPolicyIgnoreWarnings(t *testing.T) { mock := &mockTableAdminClock{} c := setupTableClient(t, mock) - err := c.SetGCPolicyIgnoreWarnings(context.Background(), "My-table", "cf1", NoGcPolicy()) + err := c.SetGCPolicy(context.Background(), "My-table", "cf1", NoGcPolicy()) if err != nil { - t.Fatalf("Failed to set GC Policy: %v", err) + t.Fatalf("SetGCPolicy: Failed to set GC Policy: %v", err) } modColumnReq := mock.modColumnReq - if !modColumnReq.IgnoreWarnings { - t.Errorf("IgnoreWarnings should be set to true") + if modColumnReq.IgnoreWarnings { + t.Errorf("SetGCPolicy: IgnoreWarnings should be set to false") } } From aa504fed9f255e31a5a7ab37938fc9cd2f16c63a Mon Sep 17 00:00:00 2001 From: Baha Aiman Date: Tue, 7 May 2024 08:15:18 +0000 Subject: [PATCH 3/5] refactor(bigtable): Change options struct to interface --- bigtable/admin.go | 48 ++++++++++++++++++++++++++++++------------ bigtable/admin_test.go | 6 +++--- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/bigtable/admin.go b/bigtable/admin.go index 37667b15b741..bcbf1ebee8cd 100644 --- a/bigtable/admin.go +++ b/bigtable/admin.go @@ -547,34 +547,56 @@ func (ac *AdminClient) TableInfo(ctx context.Context, table string) (*TableInfo, return ti, nil } -// SetGCPolicy specifies which cells in a column family should be garbage collected. -// GC executes opportunistically in the background; table reads may return data -// matching the GC policy. -func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, opts GCPolicyOptions) error { +type gcPolicySettings struct { + ignoreWarnings bool +} + +// GCPolicyOption is the interface to change GC policy settings +type GCPolicyOption interface { + apply(s *gcPolicySettings) +} + +type withIgnoreWarnings struct{ ignoreWarnings bool } + +func (w withIgnoreWarnings) apply(s *gcPolicySettings) { + s.ignoreWarnings = w.ignoreWarnings +} + +// WithIgnoreWarnings returns a gcPolicyOption that ignores safety checks when modifying the column families +func WithIgnoreWarnings() GCPolicyOption { + return withIgnoreWarnings{ignoreWarnings: true} +} + +func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, opts ...GCPolicyOption) error { ctx = mergeOutgoingMetadata(ctx, ac.md) prefix := ac.instancePrefix() + + s := gcPolicySettings{} + for _, opt := range opts { + opt.apply(&s) + } req := &btapb.ModifyColumnFamiliesRequest{ Name: prefix + "/tables/" + table, Modifications: []*btapb.ModifyColumnFamiliesRequest_Modification{{ Id: family, Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{Update: &btapb.ColumnFamily{GcRule: policy.proto()}}, }}, - IgnoreWarnings: opts.IgnoreWarnings, + IgnoreWarnings: s.ignoreWarnings, } _, err := ac.tClient.ModifyColumnFamilies(ctx, req) return err } -type GCPolicyOptions struct { - IgnoreWarnings bool -} - -func (ac *AdminClient) SetGCPolicyWithOptions(ctx context.Context, table, family string, policy GCPolicy, opts GCPolicyOptions) error { - return ac.setGCPolicy(ctx, table, family, policy, opts) +// SetGCPolicy specifies which cells in a column family should be garbage collected. +// GC executes opportunistically in the background; table reads may return data +// matching the GC policy. +func (ac *AdminClient) SetGCPolicy(ctx context.Context, table, family string, policy GCPolicy) error { + return ac.SetGCPolicyWithOptions(ctx, table, family, policy) } -func (ac *AdminClient) SetGCPolicy(ctx context.Context, table, family string, policy GCPolicy) error { - return ac.setGCPolicy(ctx, table, family, policy, GCPolicyOptions{}) +// SetGCPolicyWithOptions is similar to SetGCPolicy but allows passing options +func (ac *AdminClient) SetGCPolicyWithOptions(ctx context.Context, table, family string, policy GCPolicy, opts ...GCPolicyOption) error { + return ac.setGCPolicy(ctx, table, family, policy, opts...) } // DropRowRange permanently deletes a row range from the specified table. diff --git a/bigtable/admin_test.go b/bigtable/admin_test.go index 395d828499de..3d822daf6063 100644 --- a/bigtable/admin_test.go +++ b/bigtable/admin_test.go @@ -317,17 +317,16 @@ func TestTableAdmin_UpdateTableDisableChangeStream(t *testing.T) { func TestTableAdmin_SetGcPolicy(t *testing.T) { for _, test := range []struct { desc string - opts GCPolicyOptions + opts GCPolicyOption want bool }{ { desc: "IgnoreWarnings: false", - opts: GCPolicyOptions{}, want: false, }, { desc: "IgnoreWarnings: true", - opts: GCPolicyOptions{IgnoreWarnings: true}, + opts: WithIgnoreWarnings(), want: true, }, } { @@ -357,6 +356,7 @@ func TestTableAdmin_SetGcPolicy(t *testing.T) { modColumnReq := mock.modColumnReq if modColumnReq.IgnoreWarnings { t.Errorf("SetGCPolicy: IgnoreWarnings should be set to false") + } } func TestTableAdmin_CreateAuthorizedView_DeletionProtection_Protected(t *testing.T) { From 99de85a612d8c30bf42b695cb8266da1704e691b Mon Sep 17 00:00:00 2001 From: Baha Aiman Date: Tue, 7 May 2024 10:43:27 +0000 Subject: [PATCH 4/5] refactor(bigtable): Add nil checks and rename --- bigtable/admin.go | 16 +++++++++------- bigtable/admin_test.go | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/bigtable/admin.go b/bigtable/admin.go index bcbf1ebee8cd..ac5d9b5c8e0e 100644 --- a/bigtable/admin.go +++ b/bigtable/admin.go @@ -556,15 +556,15 @@ type GCPolicyOption interface { apply(s *gcPolicySettings) } -type withIgnoreWarnings struct{ ignoreWarnings bool } +type warnings struct{ ignore bool } -func (w withIgnoreWarnings) apply(s *gcPolicySettings) { - s.ignoreWarnings = w.ignoreWarnings +func (w warnings) apply(s *gcPolicySettings) { + s.ignoreWarnings = w.ignore } -// WithIgnoreWarnings returns a gcPolicyOption that ignores safety checks when modifying the column families -func WithIgnoreWarnings() GCPolicyOption { - return withIgnoreWarnings{ignoreWarnings: true} +// IgnoreWarnings returns a gcPolicyOption that ignores safety checks when modifying the column families +func IgnoreWarnings() GCPolicyOption { + return warnings{ignore: true} } func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, opts ...GCPolicyOption) error { @@ -573,7 +573,9 @@ func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, po s := gcPolicySettings{} for _, opt := range opts { - opt.apply(&s) + if opt != nil { + opt.apply(&s) + } } req := &btapb.ModifyColumnFamiliesRequest{ Name: prefix + "/tables/" + table, diff --git a/bigtable/admin_test.go b/bigtable/admin_test.go index 3d822daf6063..93a40e7af8d5 100644 --- a/bigtable/admin_test.go +++ b/bigtable/admin_test.go @@ -326,7 +326,7 @@ func TestTableAdmin_SetGcPolicy(t *testing.T) { }, { desc: "IgnoreWarnings: true", - opts: WithIgnoreWarnings(), + opts: IgnoreWarnings(), want: true, }, } { From 9367898d124e89a0a0b98a2b30d5a6e2182e9770 Mon Sep 17 00:00:00 2001 From: Baha Aiman Date: Tue, 7 May 2024 22:08:42 +0000 Subject: [PATCH 5/5] refactor(bigtable): Remove new struct declaration --- bigtable/admin.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bigtable/admin.go b/bigtable/admin.go index ac5d9b5c8e0e..35317c8e6d0c 100644 --- a/bigtable/admin.go +++ b/bigtable/admin.go @@ -556,15 +556,15 @@ type GCPolicyOption interface { apply(s *gcPolicySettings) } -type warnings struct{ ignore bool } +type ignoreWarnings bool -func (w warnings) apply(s *gcPolicySettings) { - s.ignoreWarnings = w.ignore +func (w ignoreWarnings) apply(s *gcPolicySettings) { + s.ignoreWarnings = bool(w) } // IgnoreWarnings returns a gcPolicyOption that ignores safety checks when modifying the column families func IgnoreWarnings() GCPolicyOption { - return warnings{ignore: true} + return ignoreWarnings(true) } func (ac *AdminClient) setGCPolicy(ctx context.Context, table, family string, policy GCPolicy, opts ...GCPolicyOption) error {