Akanksha-kedia commented on PR #18897:
URL: https://github.com/apache/pinot/pull/18897#issuecomment-4855358772
## 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`:
```java
// 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:
```java
@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.
--
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]
