deprecating vparquet v1 (grafana#3377)

* deprecating vparquet v1 * update docs and tests * meow * remove line
yvrhdn · Feb 26, 2024 · 92b8c8e · 92b8c8e
1 parent e53cee9
commit 92b8c8e
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -59,6 +59,7 @@
 * [BUGFIX] Fix TLS when GRPC is enabled on HTTP [#3300](https://github.com/grafana/tempo/pull/3300) (@joe-elliott)
 * [BUGFIX] Correctly return 400 when max limit is requested on search. [#3340](https://github.com/grafana/tempo/pull/3340) (@joe-elliott)
 * [BUGFIX] Fix autocomplete filters sometimes returning erroneous results. [#3339](https://github.com/grafana/tempo/pull/3339) (@joe-elliott)
+* [CHANGE] **Breaking Change** Deprecating vParquet v1 [#3377](https://github.com/grafana/tempo/pull/3377) (@ie-pham)
 
 ## v2.3.1 / 2023-11-28
 

diff --git a/docs/sources/tempo/configuration/_index.md b/docs/sources/tempo/configuration/_index.md
@@ -1037,7 +1037,7 @@ storage:
 
         # block configuration
         block:
-            # block format version. options: v2, vParquet, vParquet2, vParquet3
+            # block format version. options: v2, vParquet2, vParquet3
             [version: <string> | default = vParquet3]
 
             # bloom filter false positive rate.  lower values create larger filters but fewer false positives

diff --git a/docs/sources/tempo/configuration/parquet.md b/docs/sources/tempo/configuration/parquet.md
@@ -32,14 +32,14 @@ You can still use the previous format `vParquet2`.
 To enable it, set the block version option to `vParquet2` in the Storage section of the configuration file.
 
 ```yaml
-# block format version. options: v2, vParquet, vParquet2, vParquet3
+# block format version. options: v2, vParquet2, vParquet3
 [version: vParquet2]
 ```
 
 In some cases, you may choose to disable Parquet and use the old `v2` block format. Using the `v2` block format disables all forms of search, but also reduces resource consumption, and may be desired for a high-throughput cluster that does not need these capabilities. To make this change, set the block version option to `v2` in the Storage section of the configuration file.
 
 ```yaml
-# block format version. options: v2, vParquet, vParquet2, vParquet3
+# block format version. options: v2, vParquet2, vParquet3
 [version: v2]
 ```
 

diff --git a/tempodb/config_test.go b/tempodb/config_test.go
@@ -6,6 +6,7 @@ import (
 
 	"github.com/grafana/tempo/tempodb/encoding/common"
 	"github.com/grafana/tempo/tempodb/wal"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -92,7 +93,7 @@ func TestValidateConfig(t *testing.T) {
 		{
 			cfg: &Config{
 				WAL: &wal.Config{
-					Version: "vParquet",
+					Version: "vParquet2",
 				},
 				Block: &common.BlockConfig{
 					IndexDownsampleBytes: 1,
@@ -104,7 +105,7 @@ func TestValidateConfig(t *testing.T) {
 			},
 			expectedConfig: &Config{
 				WAL: &wal.Config{
-					Version: "vParquet",
+					Version: "vParquet2",
 				},
 				Block: &common.BlockConfig{
 					IndexDownsampleBytes: 1,
@@ -127,6 +128,86 @@ func TestValidateConfig(t *testing.T) {
 	}
 }
 
+func TestDeprecatedVersions(t *testing.T) {
+	errorMessage := "this version of vParquet has been deprecated, please use vParquet2 or higher"
+	tests := []struct {
+		cfg            *Config
+		expectedConfig *Config
+		err            string
+	}{
+		// block version not copied to wal if populated
+		{
+			cfg: &Config{
+				WAL: &wal.Config{
+					Version: "vParquet2",
+				},
+				Block: &common.BlockConfig{
+					IndexDownsampleBytes: 1,
+					IndexPageSizeBytes:   1,
+					BloomFP:              0.01,
+					BloomShardSizeBytes:  1,
+					Version:              "vParquet3",
+				},
+			},
+			expectedConfig: &Config{
+				WAL: &wal.Config{
+					Version: "vParquet2",
+				},
+				Block: &common.BlockConfig{
+					IndexDownsampleBytes: 1,
+					IndexPageSizeBytes:   1,
+					BloomFP:              0.01,
+					BloomShardSizeBytes:  1,
+					Version:              "vParquet3",
+				},
+			},
+		},
+		{
+			cfg: &Config{
+				WAL: &wal.Config{
+					Version: "vParquet",
+				},
+				Block: &common.BlockConfig{
+					IndexDownsampleBytes: 1,
+					IndexPageSizeBytes:   1,
+					BloomFP:              0.01,
+					BloomShardSizeBytes:  1,
+					Version:              "v2",
+				},
+			},
+			err: errorMessage,
+		},
+		{
+			cfg: &Config{
+				WAL: &wal.Config{
+					Version: "vParquet2",
+				},
+				Block: &common.BlockConfig{
+					IndexDownsampleBytes: 1,
+					IndexPageSizeBytes:   1,
+					BloomFP:              0.01,
+					BloomShardSizeBytes:  1,
+					Version:              "vParquet",
+				},
+			},
+			err: errorMessage,
+		},
+	}
+
+	for _, test := range tests {
+		err := validateConfig(test.cfg)
+		if test.err == "" {
+			require.Equal(t, nil, err)
+		} else {
+			assert.Contains(t, err.Error(), test.err)
+		}
+
+		if test.expectedConfig != nil {
+			require.Equal(t, test.expectedConfig, test.cfg)
+		}
+	}
+}
+
 func TestValidateCompactorConfig(t *testing.T) {
 	compactorConfig := CompactorConfig{
 		MaxCompactionRange: 0,

diff --git a/tempodb/encoding/common/config.go b/tempodb/encoding/common/config.go
@@ -65,5 +65,9 @@ func ValidateConfig(b *BlockConfig) error {
 		return fmt.Errorf("positive value required for bloom-filter shard size")
 	}
 
+	if b.Version == "vParquet" {
+		return fmt.Errorf("this version of vParquet has been deprecated, please use vParquet2 or higher")
+	}
+
 	return b.DedicatedColumns.Validate()
 }
diff --git a/tempodb/encoding/versioned.go b/tempodb/encoding/versioned.go
@@ -85,7 +85,6 @@ func LatestEncoding() VersionedEncoding {
 func AllEncodings() []VersionedEncoding {
 	return []VersionedEncoding{
 		v2.Encoding{},
-		vparquet.Encoding{},
 		vparquet2.Encoding{},
 		vparquet3.Encoding{},
 	}

diff --git a/tempodb/wal/wal.go b/tempodb/wal/wal.go
@@ -13,6 +13,7 @@ import (
 	"github.com/grafana/tempo/tempodb/backend/local"
 	"github.com/grafana/tempo/tempodb/encoding"
 	"github.com/grafana/tempo/tempodb/encoding/common"
+	"github.com/grafana/tempo/tempodb/encoding/vparquet"
 )
 
 const (
@@ -36,6 +37,10 @@ type Config struct {
 }
 
 func ValidateConfig(c *Config) error {
+	if c.Version == vparquet.VersionString {
+		return fmt.Errorf("this version of vParquet has been deprecated, please use vParquet2 or higher")
+	}
+
 	if _, err := encoding.FromVersion(c.Version); err != nil {
 		return fmt.Errorf("failed to validate block version %s: %w", c.Version, err)
 	}