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.

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

Deprecate usages of AssertHelpers in codebase #7094

Closed
jackye1995 opened this issue Mar 13, 2023 · 17 comments
Closed

Deprecate usages of AssertHelpers in codebase #7094

jackye1995 opened this issue Mar 13, 2023 · 17 comments
Assignees
Labels
beginner Issues for apache iceberg beginners, enjoy to contribute ! good first issue Good for newcomers

Comments

@jackye1995
Copy link
Contributor

Feature Request / Improvement

as a part of discussions in #6977 , create an issue to track the effort.

This seems like a good beginners task, or for someone that have some spare time to remove all references.

This does not need to happen in a giant PR, can be done module by module.

Query engine

None

@jackye1995 jackye1995 added the beginner Issues for apache iceberg beginners, enjoy to contribute ! label Mar 13, 2023
@liuxiaocs7
Copy link
Member

@jackye1995 I would like to work on this. Can you assign it to me?
I think this can start after #6977 is merged, what do you think?

@jackye1995
Copy link
Contributor Author

Sure, we should be merging it very soon, I can assign this to you

@Fokko Fokko added the good first issue Good for newcomers label Mar 14, 2023
@nastra
Copy link
Contributor

nastra commented Mar 14, 2023

@liuxiaocs7 Doing this on a module-by-module basis would be a good thing, as it makes it easier for reviewers to review small PRs. Also might be worth starting in one of the modules that has a lower amount of usages of AssertHelpers (iceberg-orc can probably be combined with another module)

image

@liuxiaocs7
Copy link
Member

@liuxiaocs7 Doing this on a module-by-module basis would be a good thing, as it makes it easier for reviewers to review small PRs. Also might be worth starting in one of the modules that has a lower amount of usages of AssertHelpers (iceberg-orc can probably be combined with another module)

image

Thank you for your suggestion. I will revise it step by step.

@akshayakp97
Copy link
Contributor

Hi! I'm going to try making this change to iceberg-orc module

@akshayakp97
Copy link
Contributor

akshayakp97 commented May 1, 2023

@liuxiaocs7 I sent out a PR to remove usage of AssertHelpers. Coincidentally, it overlaps with changes from your PR. Specifically these two files - https://github.com/apache/iceberg/pull/7482/files#diff-52c0deff4e445a4ed751511025b1cf380549283a85d91f67528116d9072bb86b and https://github.com/apache/iceberg/pull/7482/files#diff-65e74287fd4b789cbc6789d0f1435ca700c121114d98823e827fd1ee76174d08

Please note that I will remove these two files from my PR.

@liuxiaocs7
Copy link
Member

Hi, @akshayakp97, sorry, I didn't notice your pr and covered ORC and MR modules in Hive module. Thanks for removing the related module in #7468.

@gzagarwal
Copy link

Is there any module that i can pick up

@nastra
Copy link
Contributor

nastra commented Oct 2, 2023

@gzagarwal you can just search through the codebase and pick up any of them, such as the ones in iceberg-aws module

@gzagarwal
Copy link

gzagarwal commented Oct 3, 2023

okay let me pick, I am working on iceberg-aws

@amshali
Copy link

amshali commented Oct 4, 2023

Working on flink module...

@gzagarwal
Copy link

okay let me pick, I am working on iceberg-aws

Shall i assume current test cases are working? on my local system they are not working so asking this question.
Is there any perquisite to work on the iceberg workspace .Some settings do i need to run to work.

@nastra
Copy link
Contributor

nastra commented Oct 4, 2023

@gzagarwal yes the tests should all be working. What issue are you seeing? https://github.com/apache/iceberg/blob/a3aff95f9e60962240b94242e24a778760bdd1d9/CONTRIBUTING.md and https://iceberg.apache.org/contribute/#building-the-project-locally should contain everything needed in order to configure the project locally

@gzagarwal
Copy link

@gzagarwal yes the tests should all be working. What issue are you seeing? https://github.com/apache/iceberg/blob/a3aff95f9e60962240b94242e24a778760bdd1d9/CONTRIBUTING.md and https://iceberg.apache.org/contribute/#building-the-project-locally should contain everything needed in order to configure the project locally

Some issue with my local system only, let me fix that first , if someone wants to take care of the changes they can pick it up.
if my workspace works locally then will come back.,

@gzagarwal
Copy link

gzagarwal commented Oct 6, 2023

@gzagarwal you can just search through the codebase and pick up any of them, such as the ones in iceberg-aws module

I am running existing testcase in iceberg-aws module . am i having some issue or its failing other places.
I am having Windows machine.where executing the test case.
TestCase Name :
TestDefaultAwsClientFactory.testS3FileIoCredentialsOverride
Exception : while running the test case

Expecting actual throwable to be an instance of:
software.amazon.awssdk.services.s3.model.S3Exception
but was:
software.amazon.awssdk.core.exception.SdkClientException: Unable to marshall request to JSON: Parameter 'Bucket' must not be null
at software.amazon.awssdk.core.exception.SdkClientException$BuilderImpl.build(SdkClientException.java:111)

@nastra
Copy link
Contributor

nastra commented Nov 13, 2023

The only modules left are Spark 3.2 + Spark 3.3

image

I don't think we want to spend time migrating those. Once Spark 3.2 + 3.3 are deprecated and removed from the codebase, we can remove AssertHelpers.

@nastra nastra closed this as completed Nov 13, 2023
@findepi
Copy link
Member

findepi commented Jun 15, 2024

follow-up to complete removal of AssertHelpers -- #10500

I don't think we want to spend time migrating those

agreed! fortunately they turned out mechanically updatable. in the PR above i did this using Intellij structural replace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner Issues for apache iceberg beginners, enjoy to contribute ! good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

8 participants