azagrebin commented on pull request #14199: URL: https://github.com/apache/flink/pull/14199#issuecomment-734198236
@XComp @StephanEwen Thanks for the reviews? > The setup logic does look a bit complex - I am wondering if it makes sense to have more of the setup logic in the > MiniCluster itself? To avoid that future changes to the HAServices need to touch this test class. > (The philosophy being that this class tests DataStream and File Source, so changes not relating to those should ideally not require to touch that class). We could do this. Alternatively, `MiniCluster/MiniClusterResource` can be configured by setting a custom `HighAvailabilityServicesFactory` to `HighAvailabilityOptions.HA_MODE` in configuration. The factory can be a separate `TestHAFactory` (we already have some). I would prefer some flexible builder for it because in this test we need something like `TestingEmbeddedHaServices` + `CompletedCheckpointStore`, the former has direct control on leadership and the latter needs to be reset after each test. One thing here is that the `TestHAFactory` has to be static to use the config option, hence, the reset of `CompletedCheckpointStore`. So if we could set the factory instance directly to `MiniCluster/MiniClusterConfiguration` that would be more flexible. We could also have a separate issue for this before and after merging this PR (or at least commit). The reason, why I do not use `MiniClusterResource` in these tests now, is that `MiniCluster` does not expose `terminateTaskManager`. Whether to expose it again may be a separate issue or at least commit. cc @tillrohrmann > What do you think about adding also a failover case also for the batch execution? Then we would have everything covered. I can do this. My thinking was there is less control over concurrency between job run/completion and the failure in batch. This might not trigger the actual test all the time. Alternatively, we could introduce some managed blocking operation in the middle of reading, e.g. with an extra map in the pipeline to sync the execution with the failure. It is more complicated but not random. > We could then rename this test from `FileTextLinesITCase` to a `FileSourceFailoverITCase`. I would still keep the same tests w/o failure to keep the coverage, hence, not sure about adding `Failover`. On the other hand, it will be mostly about failover. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
