bube commented on code in PR #254:
URL:
https://github.com/apache/flink-connector-aws/pull/254#discussion_r3843303287
##########
flink-connector-aws/flink-connector-aws-kinesis-streams/src/main/java/org/apache/flink/connector/kinesis/source/reader/KinesisShardSplitReaderBase.java:
##########
@@ -179,31 +184,47 @@ private boolean
skipUntilScheduledFetchTime(KinesisShardSplitState splitState)
}
/**
- * Schedules next fetch time, to be called immediately on the result of a
fetchRecords() call.
+ * Schedules the next fetch time. To be called immediately after a
fetchRecords() call.
Review Comment:
I agree that the sentence reads strangely. This method doesn't always
schedule a fetch, though. At the default `nonempty-records-fetch-interval` of 0
it schedules nothing on purpose, because `skipUntilScheduledFetchTime()` treats
a missing map entry as "fetch now".
That, plus your other comment on line 195:
> Can you confirm that the next opportuanity path is outsider this code and
should not be a schedule with 0 internal?
makes me think the naming of methods and fields is an issue. They do not
clearly signal that the method only potentially defers the next `fetch()` call,
or that *not* putting an entry in the `scheduledFetchTimes` map is a signal for
`fetch()` to "fetch now"
How about these re-names to clarify the design an intention of the code:
`scheduleNextFetchTime()` -> `maybeDeferNextFetch()`
`scheduleAt()` -> `deferNextFetchUntil()`
`scheduledFetchTimes` -> `fetchDeferredUntil`
And obviously updating the javadoc :)
--
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]
