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

WIP: Use localhost instead of container hostname #748

Closed
wants to merge 3 commits into from
Closed

WIP: Use localhost instead of container hostname #748

wants to merge 3 commits into from

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Dec 1, 2024 •
edited
Loading

These changes allow me to also run the tests locally.

Closes #719

@Fokko Fokko force-pushed the fd-try-localhost branch 3 times, most recently from 2dbb1cf to 0d65fc2 Compare December 2, 2024 20:16
@Fokko Fokko marked this pull request as ready for review December 2, 2024 20:17
@Fokko Fokko mentioned this pull request Dec 3, 2024
Closed
3 tasks
@liurenjie1024
Copy link
Contributor

cc @Fokko Do we still this change? I see your comment #719 (comment) here.

@Fokko
Copy link
Contributor Author

Fokko commented Dec 4, 2024

@liurenjie1024 unfortunately yes, the 172.* IPs that the library return don't work on my end.

liurenjie1024
liurenjie1024 reviewed Dec 6, 2024
View reviewed changes
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @Fokko for this pr, but I have some concerns with this approach as it may block parallel running of integration tests, would you mind waiting for some time so that I can have a try to solve the problem while allowing parallel running of integration tests?

@Fokko
Copy link
Contributor Author

Fokko commented Dec 6, 2024

@liurenjie1024 Ah, yes, that would be one of the drawbacks

@nastra Asking for a favor. Could you clone this repository, and try to run make test? I wanted to check if I'm the only one that's in this situation.

@nastra
Copy link
Contributor

nastra commented Dec 6, 2024

@Fokko you're not alone. Those tests fail for me too using native Docker on OSX due to networking:

test tests::gcs_exists has been running for over 60 seconds
test tests::gcs_read has been running for over 60 seconds
test tests::gcs_write has been running for over 60 seconds
test tests::gcs_read ... FAILED
test tests::gcs_exists ... FAILED
test tests::gcs_write ... FAILED

failures:

---- tests::gcs_read stdout ----
thread 'tests::gcs_read' panicked at crates/iceberg/tests/file_io_gcs_test.rs:67:14:
called `Result::unwrap()` on an `Err` value: error sending request for url (http://172.20.0.2:4443/storage/v1/b)

Caused by:
    0: client error (Connect)
    1: tcp connect error: Operation timed out (os error 60)
    2: Operation timed out (os error 60)

---- tests::gcs_exists stdout ----
thread 'tests::gcs_exists' panicked at crates/iceberg/tests/file_io_gcs_test.rs:67:14:
called `Result::unwrap()` on an `Err` value: error sending request for url (http://172.20.0.2:4443/storage/v1/b)

Caused by:
    0: client error (Connect)
    1: tcp connect error: Operation timed out (os error 60)
    2: Operation timed out (os error 60)

---- tests::gcs_write stdout ----
thread 'tests::gcs_write' panicked at crates/iceberg/tests/file_io_gcs_test.rs:67:14:
called `Result::unwrap()` on an `Err` value: error sending request for url (http://172.20.0.2:4443/storage/v1/b)

Caused by:
    0: client error (Connect)
    1: tcp connect error: Operation timed out (os error 60)
    2: Operation timed out (os error 60)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::gcs_exists
    tests::gcs_read
    tests::gcs_write

test result: FAILED. 0 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 75.03s

@liurenjie1024
Copy link
Contributor

Hi, @Fokko @ZENOTME helped to did some investigation and found that the reason we can't use current approach is a known limitation: https://docs.docker.com/desktop/features/networking/#per-container-ip-addressing-is-not-possible

While the method in this pr works for one integration tests, it prevents us from running integration tests in parallel in future. I have some concerns with this pr, is asking developer to use orbstack on mac a reasonable solution to you?

@Fokko
Copy link
Contributor Author

Fokko commented Dec 9, 2024

I've switched to Orbstack and it works pretty well :)

@Fokko Fokko closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liurenjie1024 liurenjie1024 liurenjie1024 left review comments

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

Successfully merging this pull request may close these issues.

Make the integration tests work on OSX
3 participants
@Fokko @liurenjie1024 @nastra