Skip to content

feat(bigtable): Add ignore_warnings flag to SetGcPolicy#9372

Merged
bhshkh merged 13 commits intogoogleapis:mainfrom
andre-sampaio:ignore-warnings-gc-policy
May 14, 2024
Merged

feat(bigtable): Add ignore_warnings flag to SetGcPolicy#9372
bhshkh merged 13 commits intogoogleapis:mainfrom
andre-sampaio:ignore-warnings-gc-policy

Conversation

@andre-sampaio
Copy link
Copy Markdown
Contributor

This exposes the flag ignore_warnings for ModifyColumnFamiliesRequest.

@andre-sampaio andre-sampaio requested review from a team February 5, 2024 21:03
@product-auto-label product-auto-label Bot added size: s Pull request size is small. api: bigtable Issues related to the Bigtable API. labels Feb 5, 2024
@bhshkh
Copy link
Copy Markdown
Contributor

bhshkh commented Feb 10, 2024

Please create an issue for this pull request describing the problem

@andre-sampaio
Copy link
Copy Markdown
Contributor Author

andre-sampaio commented Feb 26, 2024

resolves #9469

@product-auto-label product-auto-label Bot added the stale: old Pull request is old and needs attention. label Mar 7, 2024
@bhshkh
Copy link
Copy Markdown
Contributor

bhshkh commented Mar 13, 2024

If and when more fields are added to ModifyColumnFamiliesRequest, we will have to create more methods to allow setting those. Adding SetGCPolicyWithOptions which takes an options struct (with ignorewarnings bool field) instead of SetGCPolicyIgnoreWarnings would be a better option for future extendability.

@bhshkh bhshkh requested a review from gkevinzheng April 16, 2024 03:50
@bhshkh
Copy link
Copy Markdown
Contributor

bhshkh commented Apr 16, 2024

If and when more fields are added to ModifyColumnFamiliesRequest, we will have to create more methods to allow setting those. Adding SetGCPolicyWithOptions which takes an options struct (with ignorewarnings bool field) instead of SetGCPolicyIgnoreWarnings would be a better option for future extendability.

Added these changes

@gkevinzheng
Copy link
Copy Markdown
Contributor

@bhshkh Don't we usually have GCPolicyOptions be an interface with a set function and different functions that return different structs? Why aren't we using that pattern here?

@bhshkh bhshkh self-assigned this Apr 16, 2024
@andre-sampaio
Copy link
Copy Markdown
Contributor Author

Thank you for adding those changes, @bhshkh. Agree that it is better that way for extensibility

@andre-sampaio andre-sampaio requested a review from a team April 18, 2024 14:13
@andre-sampaio
Copy link
Copy Markdown
Contributor Author

@bhshkh are there more concerns regarding this change?

@bhshkh
Copy link
Copy Markdown
Contributor

bhshkh commented May 6, 2024

@bhshkh Don't we usually have GCPolicyOptions be an interface with a set function and different functions that return different structs? Why aren't we using that pattern here?

Modified the code to match the pattern:

// decodeSetting contains all the settings for decoding from spanner struct
type decodeSetting struct {
Lenient bool
}
// DecodeOptions is the interface to change decode struct settings
type DecodeOptions interface {
Apply(s *decodeSetting)
}
type withLenient struct{ lenient bool }
func (w withLenient) Apply(s *decodeSetting) {
s.Lenient = w.lenient
}
// WithLenient returns a DecodeOptions that allows decoding into a struct with missing fields in database.
func WithLenient() DecodeOptions {
return withLenient{lenient: true}
}

@bhshkh bhshkh enabled auto-merge (squash) May 7, 2024 10:44
Comment thread bigtable/admin.go
Comment thread bigtable/admin.go Outdated
@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 May 14, 2024
@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 May 14, 2024
@bhshkh bhshkh merged commit 0e6413d into googleapis:main May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API. size: s Pull request size is small. stale: old Pull request is old and needs attention.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants