pvary commented on code in PR #13340:
URL: https://github.com/apache/iceberg/pull/13340#discussion_r2163438264
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/TableMetadataCache.java:
##########
@@ -220,37 +238,59 @@ SchemaInfo getSchemaInfo() {
*/
static class SchemaInfo {
private final Map<Integer, Schema> schemas;
- private final Map<Schema, Tuple2<Schema, CompareSchemasVisitor.Result>>
lastResults;
+ private final Cache<Schema, SchemaCompareInfo> lastResults;
Review Comment:
@aiborodin, @mxm: When I was working with this part of the code, I
originally used Cache. When I did testing with the happy path (no schema
evolution, no data evolution), I have seen a non negligible overhead with the
Cache, and ended up moving to LinkedList. I remember debating the issue quite a
bit, and decided to use the LinkedList.
Sadly, I don't have the history/memory to get the exact details, and don't
have the time to reproduce the measurements. @aiborodin: I really respect all
the effort you put into this PR, but I would like to ask you to revert to the
LinkedList usage, unless we have a very compelling reason to go back using the
Cache. The other parts of the change seem good to me. When we finalize the
discussion here, I will go back and review everything, and hopefully we can get
your contribution into 1.10.0.
Thanks for all the effort/work you put into this 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: [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]
