echauchot commented on PR #23443:
URL: https://github.com/apache/flink/pull/23443#issuecomment-1802261114
> The added logic makes sense IMO.
>
> Added 2 comments, but in general I would consider separating the whole
`INFLATER_INPUT_STREAM_FACTORIES` into a new class as there are a couple
functions that uses it and seems quite detachable from `FlieInputFormats`.
>
> After a quick peek I think something like the following could work:
>
> ```java
> public class InflaterInputStreamFactories {
>
> public static void register(String fileExt,
InflaterInputStreamFactory<?> factory) { ... }
>
> public static InflaterInputStreamFactory<?> get(Path path) { ... }
>
> private static InflaterInputStreamFactory<?> get(String fileExt) { ... }
>
> @VisibleForTesting
> public static Set<String> getSupportedCompressionFormats() { ... }
> }
> ```
>
> Also, `ConcurrentHashMap` can be utilized insead of the `synchronized`
block, but other than that the current logic could be moved as is now.
>
> This probably goes beyond the current PR, but I think it worth to note it.
WDYT?
I agree with the ConcurrentHashMap suggestion. Regarding creating a class
just for wrapping a map that is used only in the FileInputFormat it seems
overkill to me. An anyway it is indeed outside of the scope of this
filesize-fix PR.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
