Fix and clean-up upsert and dedup config #15528
Conversation
Jackie-Jiang
added
documentation
incompatible
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15528 +/- ##
============================================
- Coverage 62.85% 62.82% -0.03%
- Complexity 1383 1385 +2
============================================
Files 2863 2864 +1
Lines 162450 162551 +101
Branches 24881 24894 +13
============================================
+ Hits 102101 102118 +17
- Misses 52670 52743 +73
- Partials 7679 7690 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Jackie-Jiang
force-pushed
the
fix_upsert_dedup_config
branch
from
7c8a6a0 to
f14fb08
Compare
Jackie-Jiang
force-pushed
the
fix_upsert_dedup_config
branch
3 times, most recently
from
ca484ff to
46692b4
Compare
| if (_partitionUpsertMetadataManager != null) { | ||
| UpsertContext upsertContext = _partitionUpsertMetadataManager.getContext(); | ||
| if (upsertContext.isAllowPartialUpsertConsumptionDuringCommit() | ||
| || upsertContext.getUpsertMode() != UpsertConfig.Mode.PARTIAL) { |
There was a problem hiding this comment.
hmm.. good catch, but iirc, it's checking if the upsert mode is partial upsert previously... but anyway, this check looks right now
There was a problem hiding this comment.
Yeah, I just changed it to use context as source of truth
| @@ -110,7 +109,8 @@ public void testValidateIfMergeRollupCanBeEnabledOrNot() { | |||
|
|
|||
| tableConfig = | |||
| new TableConfigBuilder(TableType.REALTIME).setTableName(RAW_TABLE_NAME).setTimeColumnName(TIME_COLUMN_NAME) | |||
| .setDedupConfig(new DedupConfig(true, HashFunction.MD5)).build(); | |||
| .setDedupConfig(new DedupConfig()) | |||
| .build(); | |||
There was a problem hiding this comment.
Done. The existing format is actually wrong. Didn't touch other part of the test
| + "TTL: {}, dedup time column: {}, table index dir: {}", getClass().getSimpleName(), _tableNameWithType, | ||
| primaryKeyColumns, hashFunction, _enablePreload, metadataTTL, dedupTimeColumn, tableIndexDir); | ||
|
|
||
| // NOTE: This field doesn't follow enablement override, and always take instance config if set to true. |
There was a problem hiding this comment.
typo in comment: ... if set to false?
There was a problem hiding this comment.
If instance config is set to true. Let me revise the comment lol
| Preconditions.checkState(!upsertConfig.isEnablePreload(), | ||
| "enableDeletedKeysCompactionConsistency and enablePreload shouldn't exist together for upsert table"); | ||
| // enableDeletedKeysCompactionConsistency shouldn't exist with preload enabled | ||
| Preconditions.checkState(upsertConfig.getPreload() != Enablement.ENABLE, |
There was a problem hiding this comment.
should we check == Enablement.DISABLE here? as iiuc, if it's non-DISABLE, then it'd use server config
There was a problem hiding this comment.
It cannot be ENABLE, but can be DEFAULT or DISABLE. After server override, we need it to be disabled
|
|
||
|
|
||
| /** Class representing upsert configuration of a table. */ | ||
| /// Class representing upsert configuration of a table. |
There was a problem hiding this comment.
This is the new javadoc format (3 / instead of 2) which is more concise
|
|
||
| @JsonPropertyDescription("Function to hash the primary key.") | ||
| private HashFunction _hashFunction = HashFunction.NONE; | ||
|
|
||
| /// Maintains the mapping of merge strategies per column. |
There was a problem hiding this comment.
actually, looks like all comments started with 3 / in this file
There was a problem hiding this comment.
Yeah. 3 / means javadoc; 2 / is regular comment
Jackie-Jiang
force-pushed
the
fix_upsert_dedup_config
branch
2 times, most recently
from
825b3e5 to
e6dbecf
Compare
Jackie-Jiang
force-pushed
the
fix_upsert_dedup_config
branch
from
e6dbecf to
68e4a4b
Compare
| @@ -44,12 +46,16 @@ public enum ConsistencyMode { | |||
| } | |||
|
|
|||
| @JsonPropertyDescription("Upsert mode.") | |||
| private Mode _mode; | |||
| private Mode _mode = Mode.FULL; | |||
There was a problem hiding this comment.
This is backward compatible during rollout restart?
There was a problem hiding this comment.
Yes. We don't allow mode to be null, so this one just simplifies the setup for test.
Bugfixes:
Enablementenum with valueENABLE,DISABLEandDEFAULTto control the enablement of a feature. ForDEFAULTenablement, use the default config from upper level (e.g. instance level)snapshotandpreloadfield asEnablementintoUpsertConfigandDedupConfigso that the value can be properly overridden. Currently there is no way to disable at table level when instance level is enabledUpsertContextandDedupContextto avoid the inconsistency of server level override and config changeCleanups:
CommonConstantsIncompatible:
Release note:
enableSnapshotandenablePreloadare deprecated and replaced withsnapshotandpreload