DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5617127743
@luozihen The bounded LRU is fine — it's exactly the fallback I offered as
an alternative to per-job scoping ("if a cross-job cache is preferred for its
simplicity, bound it ... rather than an unbounded ConcurrentHashMap"), so no
need to hold off before I weigh in — go ahead and push it.
On per-job scoping: your read of the constraint is right, and I won't ask
you to touch the engine's sink-planning path for this PR.
`TableSinkFactory`/`createSink()` genuinely has no per-job hook to thread a
pre-compiled pattern map through, and reworking that is out of scope for a
connector-level change. The bounded LRU keeps the fix local, which is the right
trade-off here.
On the snippet: the shape is correct. `new LinkedHashMap<>(16, 0.75f, true)`
with the third argument `true` gives you access-order iteration, which is what
actually makes `removeEldestEntry` evict the least-recently-used entry rather
than the oldest-inserted one (leaving that flag off is a common mistake in
hand-rolled LRUs) — capped at 256, wrapped in `Collections.synchronizedMap` for
thread safety on individual `get`/`put` calls. The same benign check-then-act
race as before still applies: two threads can both miss and both compile+cache
the same pattern under load, but `Pattern.compile()` is deterministic, so the
worst case is one redundant compile, never a correctness issue.
One thing worth a sentence in the field comment or PR description: 256 is a
reasonable default for the common case (a handful of table-matching patterns
reused across resubmissions), but if you have a sense of how many distinct
patterns a busy multi-tenant deployment might realistically configure, it's
worth noting that 256 is comfortably above that rather than picked arbitrarily.
Not a blocker.
Push it and ping me once CI is green, and I'll take a final pass.
--
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]
