voonhous commented on PR #13152: URL: https://github.com/apache/hudi/pull/13152#issuecomment-4956361017
All open threads are now addressed. Before the next round, I want to flag something about **altitude**, because I think we have collectively drifted and I am as responsible as anyone -- I answered every comment at the depth it was asked, and the document is worse for it. This RFC has grown by about a third during review, and most of that growth is not design. It is code review of code that does not exist yet: which `public static` to call, which constructor overload, whether a specific stream is closed, how many `exists()` calls a request makes. Several of those notes are *correct* and I have taken them -- but a reader coming to this RFC to evaluate **whether we should build a Hudi timeline UI, and roughly how** now has to wade through `InstantComparatorV1`-vs-`V2` map equivalence to get there. That defeats the purpose of the document. I would propose we sort review comments into two buckets: **Belongs in the RFC** -- things that change what we build or what we promise, and that are expensive to reverse once code lands: - Scope and threat model (the `/v1`-parity claim was simply false; the instant-detail route was an arbitrary-path read). - The data model (grouping by comparable action -- the original group model had rows that could never hold a completed instant). - Packaging and licensing (~890KB into five release artifacts, plus ASF LICENSE obligations). - Feasibility and risk of deferred work (the Spark UI tab rests on `private[spark]` internals; the packaging guard is net-new CI tooling). **Belongs in PR review** -- things that only change how the code is typed, and that a compiler, a test, or a reviewer will catch anyway: - Exact method/overload selection. - Stream-closing discipline at a given call-site. - Per-call cost accounting to the granularity of individual `exists()` calls. The second bucket is not unimportant -- it is just cheap to fix later and expensive to specify now, and specifying it now creates a doc that goes stale the moment the implementation deviates by one line. Concretely, I would like to **trim the implementation minutiae back out** before this merges, keeping the design decisions and the contracts (empty table returns 200; instant reads resolve against the timeline; assets do not ship in engine bundles) and dropping the API-level forensics that justify them. Happy to do that as a follow-up commit on this branch if reviewers agree. @wombatu-kun -- to be clear, this is not a complaint about your review. You read the actual code rather than the prose, and that is exactly why you caught a path-traversal read, a broken group model, and ~890KB of assets silently landing in five bundles. Those were all worth catching at RFC time. I would just rather bank the design consequences here and take the API-level details to the implementation PR, where they belong and where they can be verified by a compiler instead of by a reader. -- 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]
