fix: Accept all valid http method for IRC's ConfigResponse #3010
fix: Accept all valid http method for IRC's ConfigResponse #3010kevinjqliu merged 4 commits intoapache:mainfrom
Conversation
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
Seems like allowing PUT is more of a band-aid since this is just PUT for a single catalog like Polaris and I'm not sure what other implementations have.
The current implementation matches what the spec lists as endpoints or resource endpoints defined there: https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L2092C121-L2092C157
It does look like Java is a bit more permissive in what it allows as they use HttpCore5 to parse the methods. I wouldn't be opposed to being open to matching those or adding some more to allow strings and only use our current enums for comparison so we ignore extra payload passed in the response.
Curious to see what others think.
|
We could also add support for all possible HTTP verbs supported, like HttpCore5 here- https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/main/java/org/apache/hc/core5/http/Method.java#L50 |
|
Thanks for catching this! I think it makes sense to be liberal in what we accept when serializing the HTTP methods. Im in favor of adding all the HTTP verbs. Looks like python's http library already has implemented all the HTTP methods, https://docs.python.org/3.14/library/http.html#http.HTTPMethod |
|
could you add a test showing that PUT response can be serialized correctly? perhaps in https://github.com/apache/iceberg-python/blob/main/tests/catalog/test_rest.py |
|
i see that pyiceberg 0.11.0 is failing in polaris we might want to create a patch release with this fix |
sumanth-manchala
changed the title
|
@kevinjqliu , added all the valid Http Methods and tests |
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
Thanks for fixing @sumanth-manchala! Just one small comment here and we should be goood
|
Thanks @geruh and @kevinjqliu! |
|
Thanks for the PR @sumanth-manchala and thanks @geruh for the review |
…WU-13) PyIceberg 0.11.0 rejects Polaris 1.2.0+ responses because HttpMethod enum is missing PUT. Fixed in apache/iceberg-python#3010, pinned to merge commit 9687d08 for reproducibility. Changes: - Dockerfile: install pyiceberg from pinned git commit, add git to build deps, add smoke test for HttpMethod.PUT - setup-cluster.sh: install pyiceberg fix in dev virtualenv - test-e2e.sh: standardize UV_NO_SYNC tracking comment - pyproject.toml: exclude pyiceberg==0.11.0 to protect pip users All 4 files have TODO(pyiceberg-0.11.1) tracking comments. 🤖 Generated with Claude Code <noreply@anthropic.com>
* fix: resolve E2E infrastructure blockers - Fix POSIX shell compatibility in Polaris bootstrap job (curlimages/curl uses BusyBox sh which doesn't support [[ ]]) - Lower parallel speedup threshold from 5x to 3x to reduce CI flakiness - Add dockerd sudoers entry to devcontainer so docker-in-docker works 🤖 Generated with Claude Code <noreply@anthropic.com> * fix(pyiceberg): pin to PR #3010 commit for Polaris 1.2.0 PUT compat (WU-13) PyIceberg 0.11.0 rejects Polaris 1.2.0+ responses because HttpMethod enum is missing PUT. Fixed in apache/iceberg-python#3010, pinned to merge commit 9687d08 for reproducibility. Changes: - Dockerfile: install pyiceberg from pinned git commit, add git to build deps, add smoke test for HttpMethod.PUT - setup-cluster.sh: install pyiceberg fix in dev virtualenv - test-e2e.sh: standardize UV_NO_SYNC tracking comment - pyproject.toml: exclude pyiceberg==0.11.0 to protect pip users All 4 files have TODO(pyiceberg-0.11.1) tracking comments. 🤖 Generated with Claude Code <noreply@anthropic.com> * fix(pyiceberg): add !=0.11.0 exclusion to floe-iceberg pyproject.toml (WU-13) Gate-wiring found floe-iceberg/pyproject.toml was missing the !=0.11.0 exclusion that floe-catalog-polaris already had. Both packages depend on pyiceberg and are affected by the Polaris 1.2.0 HttpMethod incompatibility. 🤖 Generated with Claude Code <noreply@anthropic.com> * fix: E2E infrastructure blockers - devcontainer, Helm values, error report - Add devcontainer.json and init-firewall.sh for reproducible dev env - Add shellcheck to devcontainer Dockerfile for quality gates - Fix Cube API deployment probes (startupProbe, readinessProbe separation) - Fix Dagster webserver port (3000, non-root user can't bind <1024) - Fix Helm values env format (dict→list for Dagster subchart compat) - Pin Polaris to 1.2.0-incubating, Cube Store to public image - Add E2E error report documenting all 46 failures across 12 categories - Update workflow state and settings 🤖 Generated with Claude Code <noreply@anthropic.com> * fix: resolve PR review feedback — test updates and Dagster version ARGs - Update test_cube_store_image to expect public cubejs/cubestore (intentional switch) - Update test_workspace_packages to allow multiple pip install lines - Update test_export_stage to expect --all-packages (uv 0.7+ change) - Extract Dagster versions to ARGs for maintainability (review comment) 🤖 Generated with Claude Code <noreply@anthropic.com> * style: ruff format test_demo_packaging.py 🤖 Generated with Claude Code <noreply@anthropic.com> --------- Co-authored-by: floe-dev <dev@floe.io>
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
Apache Polaris on how it's configured returns some polaris's specific
PUT endpoints from the config/ API which would break the RestCatalog
with newly introduced validations on ConfigResponse
https://github.com/apache/polaris/blob/de43d1d84430547d3ff90df69afcce0bff7ede4c/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java#L1328-L1337
## Are these changes tested?
Yes, I have tested with my existing instance of Polaris service
## Are there any user-facing changes?
No
<!-- In the case of user-facing changes, please add the changelog label.
-->
cc: @geruh @kevinjqliu
---------
Co-authored-by: geruh <dru@amazon.com>
Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
3 participants
Rationale for this change
Apache Polaris on how it's configured returns some polaris's specific PUT endpoints from the config/ API which would break the RestCatalog with newly introduced validations on ConfigResponse
https://github.com/apache/polaris/blob/de43d1d84430547d3ff90df69afcce0bff7ede4c/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java#L1328-L1337
Are these changes tested?
Yes, I have tested with my existing instance of Polaris service
Are there any user-facing changes?
No
cc: @geruh @kevinjqliu