Skip to content

fix(bigtable): Allow GC updates on emulated aggregate column family#11499

Merged
bhshkh merged 2 commits into
googleapis:mainfrom
wesnel:wesnel/fix-aggregate-cf-gc-policy-update
Jan 28, 2025
Merged

fix(bigtable): Allow GC updates on emulated aggregate column family#11499
bhshkh merged 2 commits into
googleapis:mainfrom
wesnel:wesnel/fix-aggregate-cf-gc-policy-update

Conversation

@wesnel

@wesnel wesnel commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

@wesnel wesnel requested review from a team January 24, 2025 04:42
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jan 24, 2025
@wesnel wesnel mentioned this pull request Jan 24, 2025
Closed
bhshkh
bhshkh reviewed Jan 24, 2025
View reviewed changes
Comment thread bigtable/bttest/inmem.go Outdated
return nil, status.Errorf(codes.InvalidArgument, "Immutable fields 'value_type.aggregate_type' cannot be updated")
}

if newcf.valueType == nil {

@bhshkh bhshkh Jan 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition needs to be changed.

Right now, column family has only two fields that can be updated. So, it makes sense to assume that when a field is nil, it is not getting updated and the other one is. This is not future compatible in the scenario where more fields get added to column family.

google-cloud-go/bigtable/admin/apiv2/adminpb/table.pb.go

Lines 953 to 976 in a2a926d

// A set of columns within a table which share a common configuration.
type ColumnFamily struct {
state protoimpl.MessageState
sizeCache protoimpl.SizeCache
unknownFields protoimpl.UnknownFields
// Garbage collection rule specified as a protobuf.
// Must serialize to at most 500 bytes.
//
// NOTE: Garbage collection executes opportunistically in the background, and
// so it's possible for reads to return a cell even if it matches the active
// GC expression for its family.
GcRule *GcRule `protobuf:"bytes,1,opt,name=gc_rule,json=gcRule,proto3" json:"gc_rule,omitempty"`
// The type of data stored in each of this family's cell values, including its
// full encoding. If omitted, the family only serves raw untyped bytes.
//
// For now, only the `Aggregate` type is supported.
//
// `Aggregate` can only be set at family creation and is immutable afterwards.
//
// If `value_type` is `Aggregate`, written data must be compatible with:
// - `value_type.input_type` for `AddInput` mutations
ValueType *Type `protobuf:"bytes,3,opt,name=value_type,json=valueType,proto3" json:"value_type,omitempty"`
}

Instead, use the UpdateMask field. As documented here, read the updatemask from the req. Then, check if it is nil. If it is nil, assume it is updating gcrule.

@wesnel wesnel Jan 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this was extremely helpful! I've just pushed some changes that will allow for partial updates of the column family based on the UpdateMask, as well as some additional tests to validate the expected behavior of the UpdateMask in this case. Please let me know if there's anything else I need to change.

@wesnel wesnel force-pushed the wesnel/fix-aggregate-cf-gc-policy-update branch from 2bef391 to 087ebb2 Compare January 25, 2025 01:39
@wesnel wesnel requested a review from bhshkh January 25, 2025 01:42
@wesnel wesnel force-pushed the wesnel/fix-aggregate-cf-gc-policy-update branch from 087ebb2 to e52fc23 Compare January 25, 2025 01:46
@bhshkh bhshkh enabled auto-merge (squash) January 28, 2025 17:54
@bhshkh

bhshkh commented Jan 28, 2025

Copy link
Copy Markdown
Contributor

Thanks for adding update mask support!
LGTM

@bhshkh bhshkh added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jan 28, 2025
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jan 28, 2025
@bhshkh bhshkh merged commit 0682bfa into googleapis:main Jan 28, 2025
@wesnel wesnel deleted the wesnel/fix-aggregate-cf-gc-policy-update branch January 28, 2025 19:18
@release-please release-please Bot mentioned this pull request Jan 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bhshkh bhshkh bhshkh approved these changes

@igorbernstein2 igorbernstein2 Awaiting requested review from igorbernstein2

Assignees

No one assigned

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bigtable: Emulator does not allow GC policy changes on existing aggregate column family

3 participants

@wesnel @bhshkh @kokoro-team