Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
May 14, 2024

Conversation

andre-sampaio
Copy link
Contributor

This exposes the flag ignore_warnings for ModifyColumnFamiliesRequest.

@andre-sampaio andre-sampaio requested review from a team as code owners 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
Contributor

bhshkh commented Feb 10, 2024

Please create an issue for this pull request describing the problem

@andre-sampaio
Copy link
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
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
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
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
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 as a code owner April 18, 2024 14:13
@andre-sampaio
Copy link
Contributor Author

@bhshkh are there more concerns regarding this change?

@bhshkh
Copy link
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
bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
@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
8 of 10 checks passed
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