gortiz commented on code in PR #14773:
URL: https://github.com/apache/pinot/pull/14773#discussion_r1906869702
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -35,14 +34,56 @@
public class ForwardIndexConfig extends IndexConfig {
+ @Deprecated
public static final int DEFAULT_RAW_WRITER_VERSION = 2;
- public static final int DEFAULT_TARGET_MAX_CHUNK_SIZE_BYTES = 1024 * 1024;
// 1MB
- public static final String DEFAULT_TARGET_MAX_CHUNK_SIZE =
- DataSizeUtils.fromBytes(DEFAULT_TARGET_MAX_CHUNK_SIZE_BYTES);
+ @Deprecated
+ public static final String DEFAULT_TARGET_MAX_CHUNK_SIZE = "1MB";
+ @Deprecated
+ public static final int DEFAULT_TARGET_MAX_CHUNK_SIZE_BYTES = 1024 * 1024;
+ @Deprecated
public static final int DEFAULT_TARGET_DOCS_PER_CHUNK = 1000;
- public static final ForwardIndexConfig DISABLED =
- new ForwardIndexConfig(true, null, null, null, null, null, null, null);
- public static final ForwardIndexConfig DEFAULT = new Builder().build();
+
+ private static int _defaultRawIndexWriterVersion = 2;
+ private static String _defaultTargetMaxChunkSize = "1MB";
+ private static int _defaultTargetMaxChunkSizeBytes = 1024 * 1024;
+ private static int _defaultTargetDocsPerChunk = 1000;
Review Comment:
I don't think it is a good practice to store default values in mutable
static attributes. In fact having static mutable attributes is a code smell. I
guess we don't have much alternative right now, but we need to add some
instance level state that can be read/inject
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
