KevinyhZou commented on code in PR #12461:
URL: https://github.com/apache/gluten/pull/12461#discussion_r3575995302
##########
gluten-flink/runtime/src/main/java/org/apache/gluten/table/runtime/operators/GlutenSourceFunction.java:
##########
Review Comment:
**Please tighten / 收敛 `shouldCallNoMoreSplits`**
This flag is introduced mainly so the idle E2E test can keep the native task
alive (skip `task.noMoreSplits(id)` after the initial split). That need is
valid, but the current shape turns a test knob into a public production API:
```java
public void setShouldCallNoMoreSplits(boolean value) { ... }
public boolean isShouldCallNoMoreSplits() { ... }
```
and it is also propagated through `OffloadedJobGraphGenerator` /
`GlutenStreamSource`.
Risks:
1. Accidental `false` in production changes split lifecycle semantics.
2. Offload path now has to carry a UT-oriented toggle as a first-class field.
3. Naming/`setter` surface suggests general configuration, but docs say it
is for “unbounded streaming **test** scenarios”.
**Suggestions (any one is fine):**
- Prefer a constructor param or package-private / `@VisibleForTesting` API
instead of a public setter.
- Avoid wiring this through `OffloadedJobGraphGenerator` unless production
offload truly needs a non-default value (default should stay `true`).
- Longer term: derive from connector/split boundedness (unbounded Kafka →
don’t auto `noMoreSplits`) rather than a hand-set boolean.
Functionality can stay; please just shrink visibility and keep it out of the
general runtime surface.
--
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]
