gjacoby126 commented on code in PR #9452:
URL: https://github.com/apache/iceberg/pull/9452#discussion_r1462334518
##########
flink/v1.16/flink/src/main/java/org/apache/iceberg/flink/util/FlinkPackage.java:
##########
@@ -18,16 +18,44 @@
*/
package org.apache.iceberg.flink.util;
+import java.util.concurrent.atomic.AtomicReference;
import org.apache.flink.streaming.api.datastream.DataStream;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
public class FlinkPackage {
- /** Choose {@link DataStream} class because it is one of the core Flink API.
*/
- private static final String VERSION =
DataStream.class.getPackage().getImplementationVersion();
+
+ private static final AtomicReference<String> VERSION = new
AtomicReference<>();
+ public static final String FLINK_UNKNOWN_VERSION = "FLINK-UNKNOWN-VERSION";
private FlinkPackage() {}
/** Returns Flink version string like x.y.z */
public static String version() {
- return VERSION;
+ if (null == VERSION.get()) {
Review Comment:
@stevenzwu - the cached value cannot be `static final String` because that
will make the code untestable. (`static final AtomicReference<String>` is fine,
because its internal state is still mutable.) If it's not final, it cannot be
named `VERSION` because the checkstyle config requires non-final static
variables to be camelCased.
The current implementation reflects @nastra and @pvary 's last suggestions
-- @nastra 's suggestion to use AtomicReference and @pvary 's suggestion to
consolidate to one function.
At this point we seem to have contradictory suggestions based on slightly
different aesthetic preferences since no two people write code exactly the
same. I'm fine implementing any of them, but I can't do _all_ of them.
Is the current implementation ready for commit or can we come to a consensus
on what needs to be done to get it ready?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
