SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5628357382
@luozihen Thanks for the update — the pattern-caching commit is the right
direction, and the bounded LRU shape (access-ordered `LinkedHashMap` capped at
256, wrapped in `Collections.synchronizedMap`) is fine as-is; the benign
double-compile race is not a concern since `Pattern.compile()` is
deterministic. Go ahead and push, and ping me once CI is green so I can take a
final pass. If you have a rough sense of how many distinct patterns a busy
multi-tenant deployment would configure, a one-line note in the field comment
or PR description that 256 sits comfortably above that would be nice, but it's
not a blocker.
On the rendering question: nothing looks truncated from where I'm reading,
so I wouldn't spend more time chasing that — if a specific comment seems cut
off on your end, just quote the part you're unsure about and I'll restate it.
To make the final pass quick, could you add a short per-item status for the
earlier findings so I'm not guessing what the latest commit covers?
- **F1 (ReadonlyConfig.toConfig() dotted-key round-trip)** — is the
API-level behavior change still in the PR, or has it been narrowed to the JDBC
sink? This is the one I most want to confirm.
- **F2 / F5 (declaration-order semantics)** — with the cached patterns, how
is "first pattern wins" now guaranteed given the option is a `Map<String,
Object>`? Either an order-preserving structure or a doc change to match actual
behavior works; please say which one you chose.
- **F3 (resolved key columns flowing into `PRIMARY_KEYS` / generated SQL)**
— is there identifier validation on the resolved column names now?
- **F4 (`${primary_key}`/`${unique_key}` vs. engine-level
TablePlaceholder)** — a sentence on which layer expands what, and in what
order, would settle this.
- **F6 (regex validation → JDBC-12)** — I assume the cache compiles once;
does an invalid pattern now fail fast at factory time with the JDBC-12 error
code?
- **F7 (tests)** — is there a regression test for the old `toConfig()`
dotted-key expansion, and an IT/E2E covering the new option?
- **F8 (`multi-table_config` naming)** — has the key been made consistent
snake_case?
Just "done / not yet / intentionally unchanged because …" per item is
plenty. Thanks again for sticking with it.
<!-- streview-comment:949 -->
--
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]
