Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade to Apache Iceberg 1.6.1 #7

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Upgrade to Apache Iceberg 1.6.1 #7

merged 1 commit into from
Sep 11, 2024

Conversation

jbonofre
Copy link
Member

Fixes #5

RussellSpitzer
RussellSpitzer previously approved these changes Jul 30, 2024
View reviewed changes
@jbonofre
Copy link
Member Author

@RussellSpitzer afair some tests were failing, I gonna fix that 😄

@flyrain
Copy link
Contributor

flyrain commented Jul 30, 2024 •
edited
Loading

This could be dangerous based on how much Polaris depends on Iceberg and the changes between iceberg 1.5 and 1.6. I'd suggest people who are familiar with the Polaris tests coverage to take a look. Just in case we hit any issue, esp. sedes related ones.

@jbonofre
Copy link
Member Author

@flyrain I'm investigating, I think I found the problem 😄

eric-maynard pushed a commit to eric-maynard/polaris that referenced this pull request Jul 30, 2024
@sfc-gh-mcollado
Mcollado ci (apache#7)
e92da9b 
* Add initial integration tests

* Add gradle workflow in github

* Fixed Dockerfile to create default-realm dir for sqlite

* Add docker-compose and Dockerfile for regtest

* Docker build in ci

* Fix context in docker-compose file

* Update regtest to work locally and in docker

* Fix dockerfile to run gradle build

* Add .keep file for output directory
eric-maynard pushed a commit to eric-maynard/polaris that referenced this pull request Jul 30, 2024
@sfc-gh-emaynard
SNOW-1527866: Implement Polaris CLI (apache#7)
905603d 
<!-- Please describe your change here and remove this comment -->

This PR implements a basic CLI for Polaris, supporting simple commands
like:
```
polaris catalogs list
polaris catalogs create --type --storage-type s3 --default-base-location s3://my-bucket --role-arn ${ARN}
polaris principals update emaynard --property foo=bar --property e=mc2
polaris privileges --catalog my_cat --catalog-role my_role namespace grant --namespace a.b.c TABLE_READ_DATA
polaris privileges --catalog my_cat --catalog-role my_role table revoke --namespace a.b.c --table t1 TABLE_READ_DATA
```

See [this
doc](https://docs.google.com/document/d/1yug1YGcNASmiOyZCJBoWXbsF5KDtO-IWQq0LLvOADzg/edit)
for a full list of supported operations.

## Pre-review checklist
- [ ] I attest that this change meets the bar for low risk without
security requirements as defined in the [Accelerated Risk Assessment
Criteria](https://developer-handbook.m1.us-west-2.aws.app.snowflake.com/docs/reference/security-review/accelerated-risk-assessment/#eligibility)
and I have taken the [Risk Assessment Training in
Workday](https://wd5.myworkday.com/snowflake/learning/course/6c613806284a1001f111fedf3e4e0000).
- Checking this checkbox is mandatory if using the [Accelerated Risk
Assessment](https://developer-handbook.m1.us-west-2.aws.app.snowflake.com/docs/reference/security-review/accelerated-risk-assessment/)
to risk assess the changes in this Pull Request.
- If this change does not meet the bar for low risk without security
requirements (as confirmed by the peer reviewers of this pull request)
then a [formal Risk
Assessment](https://developer-handbook.m1.us-west-2.aws.app.snowflake.com/docs/reference/security-review/risk-assessment/)
must be completed. Please note that a formal Risk Assessment will
require you to spend extra time performing a security review for this
change. Please account for this extra time earlier rather than later to
avoid unnecessary delays in the release process.
- [ ] This change has code coverage for the new code added
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Aug 30, 2024
@jbonofre
Copy link
Member Author

I'm resuming my work on this one, also rebasing.

@jbonofre jbonofre changed the title Upgrade to Apache Iceberg 1.6.0 Upgrade to Apache Iceberg 1.6.1 Aug 30, 2024
@jbonofre jbonofre removed the Stale label Aug 30, 2024
@jbonofre
Copy link
Member Author

jbonofre commented Sep 4, 2024

I found that the tests failure are related to this change in Apache Iceberg:

commit 24d26b6a35a1287531e72357691d9dbd3d7f79bd                                                                                                                                                                                                                                 
Author: dongwang <mingwbd@gmail.com>                                                                                                                                                                                                                                            
Date:   Fri Jul 5 15:08:44 2024 +0800                                                                                                                                                                                                                                           
                                                                                                                                                                                                                                                                                
    Core: Fix create v1 table on REST Catalog (#10369)                                                                                                                                                                                                                          
                                                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                                                
    Co-authored-by: Amogh Jahagirdar <amoghj@apache.org>

I suspect it's about the table version. I will update Polaris to deal with this Iceberg fix.

@jbonofre
Copy link
Member Author

jbonofre commented Sep 9, 2024 •
edited
Loading

I confirm commit 24d26b6a35a1287531e72357691d9dbd3d7f79bd introduce the tests failure.
The problem is that the table format version is 2 by default now.

@dimas-b
Copy link
Contributor

dimas-b commented Sep 10, 2024

@jbonofre : I believe the test failures in PolarisRestCatalogIntegrationTest are mere catalog name clashes because the new iceberg test is parameterized, which is not expected by PolarisRestCatalogIntegrationTest.before(), which uses currentCatalogName = method.getName().

I was thinking of just adding a counter to the method name.

We could try and delete old catalogs after each test, but it seems to be too involved for an Iceberg version bump due to emptiness checks on delete.

@jbonofre
Copy link
Member Author

@dimas-b it's both. I saw the namespace issue but I can confirm that the default version in table metadata cause tests failure as well.

@dimas-b
Copy link
Contributor

dimas-b commented Sep 10, 2024

hmmm.... I ran that test locally yesterday and it passed with just fixing the catalog name to be unique.

@jbonofre
Copy link
Member Author

@dimas-b let me update this PR with the fix (I would love to have your review 😄 ).

@jbonofre
Copy link
Member Author

@dimas-b you were right as the failure was related: it was mixing metadata from different catalog. By isolating catalog, it should be ok now.

dimas-b
dimas-b approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :) Thanks, @jbonofre !

@dimas-b
Copy link
Contributor

dimas-b commented Sep 11, 2024

CC: @snazy for a binding +1 :)

@snazy snazy merged commit cc58730 into main Sep 11, 2024
3 checks passed
@snazy snazy deleted the jbonofre-5 branch September 11, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimas-b dimas-b dimas-b approved these changes

@snazy snazy snazy approved these changes

@RussellSpitzer RussellSpitzer RussellSpitzer left review comments

@ashvina ashvina Awaiting requested review from ashvina ashvina is a code owner

@vvcephei vvcephei Awaiting requested review from vvcephei vvcephei is a code owner

@takidau takidau Awaiting requested review from takidau takidau is a code owner

@jackye1995 jackye1995 Awaiting requested review from jackye1995 jackye1995 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to Apache Iceberg 1.6.0
5 participants
@jbonofre @flyrain @dimas-b @RussellSpitzer @snazy