tanmayrauth commented on PR #1596: URL: https://github.com/apache/iceberg-go/pull/1596#issuecomment-5183890005
Thanks @laskoviymishka for the thorough review, all points addressed. TotalDuration now brackets only the commit loop: commitStart moved to just before the retry loop and the elapsed time is snapped right after the success break, before orphan cleanup and deleteOldMetadata. Populated the four manifest fields (manifests-created/replaced/kept and entries-processed → manifest-entries-processed) from the existing summary keys and dropped the stale "doesn't yet populate manifest counts" comment. The emit is now wrapped in a safeReport helper that recovers and logs so a panicking third-party reporter can never fail an already-committed commit — and since Scan.Reporter() also hands back the user's reporter unwrapped, I folded the scan path into the same helper rather than deferring it. The non-default-branch case now skips emission when SnapshotByName returns nil instead of silently falling back to CurrentSnapshot(), so it can't attribute the wrong snapshot. attemptsUsed is now int64 (initialized to 1, set via int64(attempt)+1) to avoid the 32-bit wrap and the 0-leak trap, with buildCommitReport taking int64 directly. On tests: the metadata-only test resets the sink up front so it no longer depends on ordering; the unit test now require.NotNil-guards every .Value deref and asserts the removed-files-size round-trip plus all four manifest fields; and I added a test that injects one retryable ErrCommitFailed and asserts Attempts == 2 so the retry-count path is actually exercised. Build, go vet, golangci-lint, and the full table/metrics suites pass under -race -shuffle=on. -- 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]
