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

[HUDI-7466] Add tests to AWSGlueCatalogSyncClient #10897

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

VitoMakarevich
Copy link
Contributor

Change Logs

Continuation of #10604
Scope:

  1. Integration test
  2. .excludeColumnSchema(true) when fetching all partitions to reduce potential overhead

Impact

Describe any public API or user-facing feature change or any performance impact.
No

Risk level (write none, low medium or high below)

If medium or high, explain what verification was done to mitigate the risks.
No

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".

  • The config description must be updated if new configs are added or the default value of the configs are changed
  • Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
    ticket number here and follow the instruction to make
    changes to the website.

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@github-actions github-actions bot added the size:S PR with lines of changes in (10, 100] label Mar 20, 2024
@VitoMakarevich
Copy link
Contributor Author

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Hudi 1.0.0-SNAPSHOT:
[INFO] 
[INFO] Hudi ............................................... SUCCESS [  4.304 s]
[INFO] hudi-tests-common .................................. SUCCESS [  5.199 s]
[INFO] hudi-io ............................................ SUCCESS [  7.691 s]
[INFO] hudi-hadoop-common ................................. SUCCESS [  5.569 s]
[INFO] hudi-common ........................................ SUCCESS [17:24 min]
[INFO] hudi-hadoop-mr ..................................... SUCCESS [01:34 min]
[INFO] hudi-sync-common ................................... SUCCESS [  9.393 s]
[INFO] hudi-hive-sync ..................................... SUCCESS [  01:14 h]
[INFO] hudi-aws ........................................... SUCCESS [16:09 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:49 h (Wall Clock)
[INFO] Finished at: 2024-03-21T04:54:20+01:00

Unfortunately, Azure looks to be blocked as before -
@Disabled("HUDI-7475 The tests do not work. Disabling them to unblock Azure CI")

@parisni if it's ok - can I put @Disabled back and we can merge?

}

@Test
public void testReadFromList() throws ExecutionException, InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe keep coherent way to name the tests among all methods ? test...should... is more meaningful to me than the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will rename

@parisni
Copy link
Contributor

parisni commented Mar 23, 2024

if it's ok - can I put @disabled back and we can merge?

how do you run the integrations tests locally ? From what I saw here they shouldn't pass. Below is how I do:

JAVA_HOME=/usr/lib/jvm/java-8-openjdk-arm64/ mvn verify -Pintegration-tests -DskipITs=false  -Dspark3.3 -Dscala-2.12 -Drat.skip=false   -pl hudi-aws

@VitoMakarevich
Copy link
Contributor Author

I spent few hours trying to make it work best, at the end surrendered, just run locally moto with docker run and run tests through IDE(screen attached)

@parisni
Copy link
Contributor

parisni commented Mar 23, 2024

just run locally moto with docker run and run tests through IDE(screen attached)

what about the command line I provided. I can take a look on your branch if needed

@VitoMakarevich
Copy link
Contributor Author

I don't think it's critical, I mean this way or another it passes

@parisni
Copy link
Contributor

parisni commented Mar 24, 2024

From the error logs you have

Error:  DOCKER> [motoserver/moto:5.0.3] "it-aws": Timeout after 10060 ms while waiting on url http://localhost:5010/moto-api/
Error:  DOCKER> Error occurred during container startup, shutting down...

I cloned your branch and reproduce the error locally when running the IT tests.

Turns out you should use port 5000 and not 5010. If you configure the container to the latter, you have to configure moto to use the same. But currently moto listen the former.

In my case, falling back to 5000 port fixed the error, should be the same on azure CI

@VitoMakarevich
Copy link
Contributor Author

The log of my tests:

  1. Saw this test - submitted first PR without tests, then @parisni asked for it.
  2. Checked that tests were disabled, so indeed tried to make them work on CI - jus removed @Disabled annotation - it failed
  3. Tried with another port on CI(5010)
  4. Spent about 2 hours trying to make it work
  5. Disabled it back and reverted changes(@Disabled and port).

I can try to just enable it as it's how was before, I'm not familiar with the CI part here, so decided just to ensure tests work as not want to spend much time

@VitoMakarevich
Copy link
Contributor Author

with all defaults i see java.util.concurrent.ExecutionException: software.amazon.awssdk.core.exception.SdkClientException: Unable to execute HTTP request: Connection refused: localhost/127.0.0.1:5000

@CTTY
Copy link
Contributor

CTTY commented Mar 25, 2024

Is PR this still WIP?

@VitoMakarevich
Copy link
Contributor Author

Now it should pass, I'm not familiar with CI setup to fix it unfortunately - at least it was like this before, so it's not completely fixed but definitely it's better than before and there is no risk.

@VitoMakarevich
Copy link
Contributor Author

@hudi-bot run azure

@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

Copy link
Contributor

@parisni parisni left a comment

Choose a reason for hiding this comment

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

Disabling tests is not a way to provide tests. If you are stuck please let me know and I can bring a patch

@VitoMakarevich
Copy link
Contributor Author

If you open patchset you'll see it's not me who disabled them
It was done here #10821.
Can we make repairing tests a separate effort? It's not in the scope of this PR, I understand that this improvement is important, but the initial goal is different.

@parisni
Copy link
Contributor

parisni commented Apr 1, 2024

Can we make repairing tests a separate effort?

makes sense. thanks for your insight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:S PR with lines of changes in (10, 100]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants