yuqi1129 commented on PR #13126: URL: https://github.com/apache/gravitino/pull/13126#issuecomment-5665252016
Reviewed `1f93c01943`. The explicit advertised URI is a good fit here: the public Iceberg REST endpoint cannot reliably be inferred from the Gravitino request's proxy headers when the two services have different hosts or paths. Keeping the existing service/provider/metalake checks before applying the override, and preserving the listener-derived fallback, looks correct. I found no blocking issue. A few non-blocking suggestions: 1. **Validate explicit port ranges.** [`isValidAdvertisedUri`](https://github.com/apache/gravitino/blob/1f93c019434724aa23a9c0b379ebbb427b0f419e/server/src/main/java/org/apache/gravitino/server/web/rest/IcebergRESTServiceOperations.java#L196) accepts `https://iceberg.example.com:70000/iceberg`: Java `URI` parses it successfully, and the current checks return true. Discovery then advertises an unusable endpoint instead of reporting the configuration error. Consider allowing an omitted port, but requiring an explicit port to be in `1–65535`, with a regression test. 2. **Document all validation constraints.** The Iceberg REST service docs currently mention only an absolute HTTP/HTTPS URI. The implementation also requires a non-blank host and rejects queries and fragments. Please state those constraints explicitly so a value such as `https://iceberg.example.com/iceberg?x=1` is clearly unsupported before an operator encounters a discovery 500. 3. **Strengthen the advertised-URI test.** The test comment says the public scheme, host, port, and path all differ from the listener, but the example still uses `/iceberg/`. An endpoint such as `https://iceberg.example.com:8443/proxy/iceberg/` would explicitly cover preservation of a custom port, proxy path prefix, and trailing slash. It would also be useful to cover an advertised URI configured while the auxiliary service is unregistered; the current early-return ordering already appears correct. Validation for this review: source inspection and a standalone probe of the URI validation logic, including the out-of-range port case. I did not rerun the full test suite locally. -- 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]
