Support segmentNamePostfix in segment name #7646
Conversation
| @@ -165,7 +165,8 @@ public void setup(Context context) | |||
| new NormalizedDateSegmentNameGenerator(_rawTableName, _jobConf.get(JobConfigConstants.SEGMENT_NAME_PREFIX), | |||
| _jobConf.getBoolean(JobConfigConstants.EXCLUDE_SEQUENCE_ID, false), | |||
| IngestionConfigUtils.getBatchSegmentIngestionType(_tableConfig), | |||
| IngestionConfigUtils.getBatchSegmentIngestionFrequency(_tableConfig), dateTimeFormatSpec); | |||
| IngestionConfigUtils.getBatchSegmentIngestionFrequency(_tableConfig), dateTimeFormatSpec, | |||
| _jobConf.get(JobConfigConstants.SEGMENT_NAME_POSTFIX)); | |||
There was a problem hiding this comment.
Would this same change need to be made in v0_deprecated/pinot-spark, pinot-batch-ingestion-common, the local support, and other places where new NormalizedDateSegmentNameGenerator() is called?
There was a problem hiding this comment.
That's actually a very good idea, while not every caller provides the postfix config. That's why I initially introduced a new constructor that keeps the same behavior as it is. Will apply the changes to those classes in the next push.
Codecov Report
@@ Coverage Diff @@
## master #7646 +/- ##
============================================
- Coverage 71.75% 71.68% -0.07%
- Complexity 3997 3999 +2
============================================
Files 1576 1576
Lines 79966 79971 +5
Branches 11843 11844 +1
============================================
- Hits 57377 57328 -49
- Misses 18730 18776 +46
- Partials 3859 3867 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
This remind me of merge/roll-up tasks, we put |
+1 on that. It's not a good idea to put timestamp as the prefix as segment names are listed in alphanumeric order. |
jackjlli
force-pushed
the
support-segmentNamePostfix-in-segment-name
branch
from
70c1d2b to
3b99468
Compare
|
|
||
| public NormalizedDateSegmentNameGenerator(String tableName, @Nullable String segmentNamePrefix, | ||
| boolean excludeSequenceId, @Nullable String pushType, @Nullable String pushFrequency, | ||
| @Nullable DateTimeFormatSpec dateTimeFormatSpec) { | ||
| @Nullable DateTimeFormatSpec dateTimeFormatSpec, @Nullable String segmentNamePostfix) { |
There was a problem hiding this comment.
Should we create a new constructor to make the class backward compatible? This change will break the external code that depends on this class (i.e. our internal offline push flow).
I'm personally fine with keeping this way as long as you make sure to update the external code accordingly.
There was a problem hiding this comment.
The external code has to adopt the new constructor in order to accept segmentNamePostfix as a parameter, so either way the external code has to be changed. We should be good here.
Co-authored-by: Jack Li(Analytics Engineering) <jlli@jlli-mn1.linkedin.biz>
Description
This PR supports segmentNamePostfix in segment name in
NormalizedDateSegmentNameGeneratorclass.Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation