DanielCarter-stack commented on PR #10552:
URL: https://github.com/apache/seatunnel/pull/10552#issuecomment-3982586126
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10552", "part": 1,
"total": 1} -->
### Issue 1: Some public methods lack Javadoc
**Location**:
- `setCollector(SeaTunnelSourceCollector<T>)` - line 127
- `sendSourceEventToEnumerator(SourceEvent)` - line 328
**Related context**:
- `setCollector()` is called by `SourceSeaTunnelTask`, used for dependency
injection
- `sendSourceEventToEnumerator()` is called by
`SourceReaderContext.sendSourceEventToEnumerator()`, ultimately used by
Connector implementations
**Problem description**:
These two public methods have no Javadoc comments. Although the PR
description doesn't claim to add Javadoc for all methods, as part of the public
API, they should be documented.
**Potential risks**:
- Low risk: these methods have relatively low usage frequency
- `setCollector()` is mainly for internal use
- `sendSourceEventToEnumerator()` is called through the
`SourceReader.Context` interface
**Impact scope**:
- Direct impact: Connector developers
- Indirect impact: developers understanding `SourceReader.Context`
implementation
- Impact scope: individual Connectors
**Severity**: MINOR
**Improvement suggestion**:
```java
/**
* Sets the collector for this source lifecycle.
*
* <p>This method is called after construction to inject the collector
dependency.
*
* @param collector the source collector to use
*/
public void setCollector(SeaTunnelSourceCollector<T> collector) {
this.collector = collector;
}
/**
* Sends a source event to the remote split enumerator.
*
* <p>This method is called by {@link
SourceReader.Context#sendSourceEventToEnumerator(SourceEvent)}
* to allow custom communication between the reader and enumerator.
*
* @param sourceEvent the event to send to the enumerator
* @throws RuntimeException if the event fails to send due to communication
errors
*/
public void sendSourceEventToEnumerator(SourceEvent sourceEvent) {
// ... existing code ...
}
```
### Issue 2: Some @Override methods lack Javadoc
**Location**:
- `close()` - line 178
- `notifyCheckpointEnd(long)` - line 483
- `restoreState(List<ActionSubtaskState>)` - line 495
**Related context**:
- `close()`: overrides `AutoCloseable.close()`, triggers `ReaderCloseEvent`
and closes reader
- `notifyCheckpointEnd()`: implements `InternalCheckpointListener`, cleans
up schema-change phase
- `restoreState()`: restores splits state from checkpoint
**Problem description**:
These three @Override methods have no Javadoc. Although the interface/parent
class already has documentation, implementation classes should typically add
additional implementation detail descriptions.
**Potential risks**:
- Low risk: interface/parent class already has basic documentation
- But lacks implementation-specific behavior descriptions (such as
schema-change phase cleanup in `notifyCheckpointEnd()`)
**Impact scope**:
- Direct impact: developers understanding checkpoint mechanism
- Impact scope: core framework
**Severity**: MINOR
**Improvement suggestion**:
```java
/**
* {@inheritDoc}
*
* <p>Fires a {@link ReaderCloseEvent} and closes the underlying {@link
SourceReader}.
*/
@Override
public void close() throws IOException {
context.getEventListener().onEvent(new ReaderCloseEvent());
reader.close();
super.close();
}
/**
* {@inheritDoc}
*
* <p>If the completed checkpoint matches an in-progress schema-change phase,
* clears the phase to resume data collection.
*/
@Override
public void notifyCheckpointEnd(long checkpointId) throws Exception {
if (schemaChangePhase.get() != null
&& schemaChangePhase.get().getCheckpointId() == checkpointId) {
log.info(
"notify schema-change checkpoint[{}] end, phase: [{}]",
checkpointId,
schemaChangePhase.get().getPhase());
schemaChangePhase.set(null);
}
}
/**
* Restores the source reader's split state from a checkpoint.
*
* <p>Deserializes the splits and sends them to the remote split enumerator
* for reassignment. This is called during job recovery from a checkpoint.
*
* @param actionStateList the list of checkpoint states to restore
* @throws Exception if state restoration fails
*/
@Override
public void restoreState(List<ActionSubtaskState> actionStateList) throws
Exception {
// ... existing code ...
}
```
--
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]
