Skip to content

Remove deprecated DEFAULT_SEGMENT_PUSH_TYPE constant#18897

Closed
Akanksha-kedia wants to merge 1 commit into
apache:masterfrom
Akanksha-kedia:cleanup/remove-deprecated-default-segment-push-type
Closed

Remove deprecated DEFAULT_SEGMENT_PUSH_TYPE constant#18897
Akanksha-kedia wants to merge 1 commit into
apache:masterfrom
Akanksha-kedia:cleanup/remove-deprecated-default-segment-push-type

Conversation

@Akanksha-kedia

Copy link
Copy Markdown
Contributor

Description

Remove the deprecated DEFAULT_SEGMENT_PUSH_TYPE constant from TableConfigBuilder. The string literal "APPEND" is now inlined at the two usage sites (field initializer and fallback branch in setSegmentPushType()). The associated TODO comment requesting this removal is also deleted.

The _segmentPushType field and setSegmentPushType() method remain in place (they are independently marked @Deprecated) so all existing callers continue to compile and behave identically.

Note: the similarly-named DEFAULT_SEGMENT_PUSH_TYPE in SegmentGenerationAndPushTaskGenerator is a separate, non-deprecated constant of type BatchConfigProperties.SegmentPushType and is intentionally left untouched.

Related Issue

Fixes #17072

Changes Made

  • Removed private static final String DEFAULT_SEGMENT_PUSH_TYPE = "APPEND" from TableConfigBuilder
  • Replaced its two usages with the inline literal "APPEND"
  • Removed the // TODO: Remove 'DEFAULT_SEGMENT_PUSH_TYPE' in the future major release. comment

Testing Done

  • mvn spotless:apply -pl pinot-spi — passes
  • mvn checkstyle:check -pl pinot-spi — passes
  • mvn license:format -pl pinot-spi — passes
  • mvn -pl pinot-spi -am test-compile — compiles cleanly

Upgrade Notes

No breaking change. The removed constant was private and had no external visibility. All public API behavior is preserved.

Remove the deprecated `DEFAULT_SEGMENT_PUSH_TYPE` constant from
`TableConfigBuilder` and inline the "APPEND" string literal at its
two usage sites. The TODO comment requesting this removal is also
deleted.

The `_segmentPushType` field and `setSegmentPushType()` method remain
(they are independently deprecated) so existing callers continue to
work unchanged.
@codecov-commenter

codecov-commenter commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.81%. Comparing base (7fe517a) to head (8b7cc48).
⚠️ Report is 300 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18897      +/-   ##
============================================
+ Coverage     63.68%   64.81%   +1.13%     
+ Complexity     1684     1347     -337     
============================================
  Files          3262     3393     +131     
  Lines        199826   211664   +11838     
  Branches      31031    33305    +2274     
============================================
+ Hits         127264   137196    +9932     
- Misses        62414    63396     +982     
- Partials      10148    11072     +924     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 64.81% <ø> (+1.13%) ⬆️
temurin 64.81% <ø> (+1.13%) ⬆️
unittests 64.81% <ø> (+1.13%) ⬆️
unittests1 57.00% <ø> (+1.24%) ⬆️
unittests2 37.18% <ø> (+2.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Akanksha-kedia

Copy link
Copy Markdown
Contributor Author

cc @Jackie-Jiang @mcvsubbu @xiangfu0 — requesting your review.

What this PR does

Removes the deprecated DEFAULT_SEGMENT_PUSH_TYPE constant from TableConfigBuilder (#17072).

The constant was @deprecated with a TODO to remove it. It was used in two places inside TableConfigBuilder (field initializer + setSegmentPushType fallback). Both now use the APPEND literal directly.

Not touched: the _segmentPushType field and setSegmentPushType() method (kept deprecated for backward compat), and the similarly named constant in SegmentGenerationAndPushTaskGenerator (different type, not deprecated).

Change scope: 3-line removal. No behavior change, no API surface change.

@Akanksha-kedia

Copy link
Copy Markdown
Contributor Author

Code Review — PR #18897 (cleanup/remove-deprecated-default-segment-push-type)

One file, purely internal refactor — direction is correct. No CRITICAL or MAJOR issues. Two minor items below.


C5.2 / C6.2 — MINOR: Asymmetry between REFRESH and APPEND creates future divergence risk

REFRESH still uses the named constant REFRESH_SEGMENT_PUSH_TYPE in both its comparison and assignment. APPEND now uses bare string literals at two sites:

  • TableConfigBuilder.java:82 — field initializer: private String _segmentPushType = "APPEND";
  • TableConfigBuilder.java:225 — else-branch: _segmentPushType = "APPEND";

If one is updated without the other in a future patch, the defaults will silently diverge. There is also no test asserting that builder.build().getValidationConfig().getSegmentPushType() equals "APPEND" by default.

Suggested fix: Anchor to the existing canonical constant in IngestionConfigUtils:

// field initializer:
private String _segmentPushType = IngestionConfigUtils.DEFAULT_SEGMENT_INGESTION_TYPE;

// else branch of setSegmentPushType():
_segmentPushType = IngestionConfigUtils.DEFAULT_SEGMENT_INGESTION_TYPE;

IngestionConfigUtils.DEFAULT_SEGMENT_INGESTION_TYPE is "APPEND" (line 53) and is already the authoritative default for ingestion type across the codebase.


C3.2 — MINOR (pre-existing, not introduced by this PR): setSegmentPushType() is missing @Deprecated annotation

The method has a Javadoc @deprecated tag but no @Deprecated annotation. IDEs and compiler warnings depend on the annotation, not the Javadoc tag, to alert callers. Worth fixing opportunistically since the method body is being touched:

@Deprecated
public TableConfigBuilder setSegmentPushType(String segmentPushType) {

Everything else looks good

  • Removed constant was private static final — no public API breakage, no rolling-upgrade concern.
  • Both inline "APPEND" literals exactly match the removed constant value.
  • No test referenced the constant by name; all existing tests compile unchanged.
  • Deferring the removal of _segmentPushType field and setSegmentPushType() method is the right call given 8+ active call sites. A follow-up issue to track that cleanup would be helpful.
  • Scope is minimal: one file, one logical change.

@Akanksha-kedia

Copy link
Copy Markdown
Contributor Author

Closing as duplicate of #17072 which covers the same change. Will consolidate into that PR and address the backward-compatibility feedback from @Jackie-Jiang there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants