Skip to content

Remove -store.fullsize-chunks option #2656

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Remove -store.fullsize-chunks option #2656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
* [CHANGE] Experimental Memberlist ring: randomize gossip node names to avoid conflicts when running multiple clients on the same host, or reusing host names (eg. pods in statefulset). Node name randomization can be disabled by using `-memberlist.randomize-node-name=false`. #2715
* [CHANGE] Memberlist KV client is no longer considered experimental. #2725
* [CHANGE] Change target flag for purger from `data-purger` to `purger` and make delete request cancellation duration configurable. #2760
* [CHANGE] Removed `-store.fullsize-chunks` option which was undocumented and unused (it broke ingester hand-overs). #2656
* [FEATURE] TLS config options added for GRPC clients in Querier (Query-frontend client & Ingester client), Ruler, Store Gateway, as well as HTTP client in Config store client. #2502
* [FEATURE] The flag `-frontend.max-cache-freshness` is now supported within the limits overrides, to specify per-tenant max cache freshness values. The corresponding YAML config parameter has been changed from `results_cache.max_freshness` to `limits_config.max_cache_freshness`. The legacy YAML config parameter (`results_cache.max_freshness`) will continue to be supported till Cortex release `v1.4.0`. #2609
* [FEATURE] Experimental gRPC Store: Added support to 3rd parties index and chunk stores using gRPC client/server plugin mechanism. #2220
Expand Down
1 change: 0 additions & 1 deletion pkg/chunk/encoding/chunk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ func TestLen(t *testing.T) {
var step = int(15 * time.Second / time.Millisecond)

func TestChunk(t *testing.T) {
alwaysMarshalFullsizeChunks = false
for _, tc := range []struct {
encoding Encoding
maxSamples int
Expand Down
6 changes: 2 additions & 4 deletions pkg/chunk/encoding/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,13 @@ type Config struct{}

var (
// DefaultEncoding exported for use in unit tests elsewhere
DefaultEncoding = Bigchunk
alwaysMarshalFullsizeChunks = true
Copy link
Contributor

@pracucci pracucci May 29, 2020

Choose a reason for hiding this comment

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

Why are you saying it was unused? Being the default true doesn't it mean it's always used unless explicitly disabled via CLI flag?

Copy link
Contributor Author

@bboreham bboreham May 29, 2020

Choose a reason for hiding this comment

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

Yes, it was written this way to avoid changing things for existing users.
I mean the option to disable was never used.

Copy link
Contributor Author

@bboreham bboreham May 29, 2020

Choose a reason for hiding this comment

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

However I got the change wrong.

Copy link
Contributor

@pracucci pracucci May 29, 2020

Choose a reason for hiding this comment

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

Yes, then we need to remove the flag while always keeping it enabled ;)

Copy link
Contributor Author

@bboreham bboreham May 29, 2020

Choose a reason for hiding this comment

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

Try now.

bigchunkSizeCapBytes = 0
DefaultEncoding = Bigchunk
bigchunkSizeCapBytes = 0
)

// RegisterFlags registers configuration settings.
func (Config) RegisterFlags(f *flag.FlagSet) {
f.Var(&DefaultEncoding, "ingester.chunk-encoding", "Encoding version to use for chunks.")
flag.BoolVar(&alwaysMarshalFullsizeChunks, "store.fullsize-chunks", alwaysMarshalFullsizeChunks, "When saving varbit chunks, pad to 1024 bytes")
flag.IntVar(&bigchunkSizeCapBytes, "store.bigchunk-size-cap-bytes", bigchunkSizeCapBytes, "When using bigchunk encoding, start a new bigchunk if over this size (0 = unlimited)")
}

Expand Down
6 changes: 2 additions & 4 deletions pkg/chunk/encoding/varbit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ func (c varbitChunk) Utilization() float64 {
}

// marshalLen returns the number of bytes that should be marshalled for this chunk
// (if someone has used a version of this code that doesn't just send 1024 every time)
func (c varbitChunk) marshalLen() int {
bits := c.nextSampleOffset()
if bits < varbitThirdSampleBitOffset {
Expand All @@ -340,10 +341,7 @@ func (c varbitChunk) Len() int {
}

func (c varbitChunk) Size() int {
if alwaysMarshalFullsizeChunks {
return cap(c)
}
return c.marshalLen()
return cap(c)
}

func (c varbitChunk) firstTime() model.Time {
Expand Down