Re: [PR] [FLINK-39588][flink-table][connector/datagen] Migrate the DataGen table connector off deprecated source APIs [flink]

["Dennis-Mircea (via GitHub)" <gi...@apache.org>]

Dennis-Mircea commented on PR #28083:
URL: https://github.com/apache/flink/pull/28083#issuecomment-4702300159

   > my AI raised these issues:
   > 
   > #### 1. already fixed
   > #### 2. **Potential Null Safety Issue**
   > **Location:** `DataGenTableSourceFactoryTest.stubReaderContext()`
   > 
   > ```java
   > private static SourceReaderContext stubReaderContext() {
   >     return null;
   > }
   > ```
   > 
   > **Concern:** Returning `null` for `SourceReaderContext` is risky. While the comment states generators don't invoke methods on the context, this creates a fragile contract:
   > 
   > * Future generator implementations might call context methods
   > * NPE would occur at runtime, not compile time
   > * No compile-time safety
   > 
   > **Recommendation:** Use a proper stub/mock implementation:
   > 
   > ```java
   > private static SourceReaderContext stubReaderContext() {
   >     return new SourceReaderContext() {
   >         @Override public int currentParallelism() { return 1; }
   >         @Override public int getIndexOfSubtask() { return 0; }
   >         // ... implement other methods with sensible defaults
   >     };
   > }
   > ```
   > 
   > #### 3. **Missing Javadoc on Public Test Methods**
   > **Location:** `DataGenTableSourceFactoryTest`
   > 
   > Several new test methods lack Javadoc:
   > 
   > * `testSequenceProducesEachValueExactlyOnce()`
   > * `testEffectiveCountIsCappedByRowCountWhenSmaller()`
   > * `testEffectiveCountIsCappedBySequenceWhenRowCountLarger()`
   > 
   > **Recommendation:** Add brief Javadoc explaining what behavior each test verifies.
   > 
   > #### 4. **Serialization Not Explicitly Tested**
   > **Observation:** The old test had explicit serialization testing:
   > 
   > ```java
   > gen = InstantiationUtil.clone(gen);
   > ```
   > 
   > The new implementation removes this. While the `serialVersionUID` fields are present, there's no explicit verification that the new generator functions are properly serializable.
   > 
   > **Recommendation:** Add a test that clones a generator function and verifies it still works:
   > 
   > ```java
   > @Test
   > void testGeneratorFunctionSerialization() throws Exception {
   >     GeneratorFunction<Long, RowData> original = buildRowGenerator(...);
   >     GeneratorFunction<Long, RowData> cloned = 
   >         InstantiationUtil.clone(original);
   >     cloned.open(stubReaderContext());
   >     // Verify cloned generator produces expected output
   > }
   > ```
   > 
   > #### 5. **Magic Number in Test**
   > **Location:** `DataGenTableSourceFactoryTest.runGenerator()`
   > 
   > ```java
   > long limit = count == Long.MAX_VALUE ? 1_000L : count;
   > ```
   > 
   > The `1_000L` is a magic number. Should be a named constant:
   > 
   > ```java
   > private static final long DEFAULT_UNBOUNDED_TEST_LIMIT = 1_000L;
   > ```
   
   Thanks @davidradl, appreciate the careful pass. Addressed below:
   
   1. I don't understand this point. What is already fixed?
   
   2. **(null `SourceReaderContext`)** - I've left this as-is. The stub is documented: the generator functions only use `open()` for one-time initialization and never call any method on the context, so a `null`-returning stub is sufficient and the contract is spelled out in the Javadoc. If a future generator starts using the context, the right move would be to switch to the existing `TestingReaderContext` from `flink-connector-test-utils` rather than hand-roll an anonymous class, but pulling that test dependency into `flink-table-api-java-bridge` isn't justified today.
   
   3. **(Javadoc on test methods)** - I'd prefer to skip this one. Flink test methods are package-private (not public) and the codebase convention is generally no Javadoc on tests. The names (`testSequenceProducesEachValueExactlyOnce`, etc.) are intended to be self-documenting.
   
   4. **(serialization not explicitly tested)** - good catch, this was the one with real value. The generator functions are shipped to the TaskManagers, so serializability matters. Added `testRowGeneratorIsSerializable`, which clones the freshly built `GeneratorFunction<Long, RowData>` via `InstantiationUtil.clone(...)` and verifies the round-tripped copy still produces the full sequence. (Note the existing `datagen` ITCases already exercise the serialization path end-to-end through a MiniCluster. This just adds cheap unit-level coverage)
   
   5. **(magic number)** - done. Extracted `1_000L` into a named constant `DEFAULT_UNBOUNDED_TEST_LIMIT` with a comment explaining when the cap applies.
   


-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org