mxm commented on code in PR #15780:
URL: https://github.com/apache/iceberg/pull/15780#discussion_r3122490026
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/DynamicRecord.java:
##########
@@ -38,6 +40,7 @@ public class DynamicRecord {
private int writeParallelism;
private boolean upsertMode;
@Nullable private Set<String> equalityFields;
+ @Nullable private FlinkWriteConf flinkWriteConf;
Review Comment:
Thread safety shouldn't change. The user does not see the wrapper. It is
safe to reuse DynamicRecord. There are multiple levels of safety:
1. We are building a new DynamicRecordInternal based on DynamicRecord (or
its wrapper)
2. All objects in DynamicRecord are immutability
3. Flink copies objects between operators by default
(1) and (2) are already sufficient.
>I will add a comment for now on DynamicRecord to ensure Wrapper overrides
new methods in future.
That would leak internal information. I understand that you want to prevent
missing new methods, but on the other hand, if those new methods are not passed
on, they are probably not relevant for the processing. Once we add new methods,
tests should ensure that they are probably passed on.
--
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]
