laskoviymishka commented on issue #1236: URL: https://github.com/apache/iceberg-go/issues/1236#issuecomment-4752725735
Thanks @tanmayrauth — assigning you. The first part — framework + REST reporter, PRs 1–6 — looks solid and pretty straightforward to execute. It’s a Go port of machinery Java has run for years, and the REST envelope is pinned by the spec, so I don’t see a reason to hold it up. Go ahead and start. I’m leaning toward starting the OTel piece (PR 7) too, even though nothing has merged on the Java side yet. I’d just cross-link #16250 as an explicit dependency so we can track it and realign if the names change before merge. A few of these are cross-implementation contracts rather than iceberg-go-local decisions, so I’d rather surface them to the community before we bake them in: 1. **OTel metric and attribute names.** The point is cross-implementation dashboard parity, but the names are copied from apache/iceberg#16250, which is still unmerged and marked provisional because there’s no OTel semantic convention yet (apache/iceberg#16169). If we ship first and Java changes them during review, the dashboards diverge instead of lining up. Same risk applies to the default attribute allowlists and the “never snapshot-id” rule. My preference is to start building, mark the OTel surface experimental (“names may change”), and not treat it as stable until apache/iceberg#16250 lands. Any objection to tracking Java here instead of defining our own names? 2. **REST report metric vocabulary.** The envelope — `ReportMetricsRequest`, the report-type discriminator, `CounterResult` / `TimerResult` — is pinned by the OpenAPI spec. The individual kebab-case metric keys are not; those are de-facto Java conventions. Copying Java is the right call, but I’d make interop tests against real Java-produced JSON a requirement, not a nice-to-have. 3. **Commit report vs snapshot summary names.** Our snapshot-summary keys already differ from Java’s commit-report vocabulary: `added-files-size` vs `added-files-size-bytes`, `added-position-deletes` vs `added-positional-deletes`, `deleted-data-files` vs `removed-data-files`. The mapping needs to match Java exactly or metrics will silently diverge across clients. I’d cover this in the same interop tests. 4. **Expression JSON.** Emitting `AlwaysTrue` in v1 is fine as a stopgap, but it’s lossy and should be documented as such. When real filter serialization lands, it needs to match Java’s `ExpressionParser.toJson` — that’s also what REST predicate pushdown will depend on. None of this blocks starting. PRs 1–6 can go now, and OTel can move in parallel as long as it stays clearly provisional and linked to apache/iceberg#16250. -- 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]
