-
Notifications
You must be signed in to change notification settings - Fork 3.8k
GH-47349: [C++] Include request ID in AWS S3 Error #47351
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
base: main
Are you sure you want to change the base?
Conversation
|
81e0329
to
182cb18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only E2E test I can seem to find that reuses this error message is:
arrow/python/pyarrow/tests/test_fs.py
Lines 1431 to 1452 in 71a7b55
@pytest.mark.s3 | |
def test_s3fs_wrong_region(): | |
from pyarrow.fs import S3FileSystem | |
# wrong region for bucket | |
# anonymous=True incase CI/etc has invalid credentials | |
fs = S3FileSystem(region='eu-north-1', anonymous=True) | |
msg = ("When getting information for bucket 'voltrondata-labs-datasets': " | |
r"AWS Error UNKNOWN \(HTTP status 301\) during HeadBucket " | |
"operation: No response body. Looks like the configured region is " | |
"'eu-north-1' while the bucket is located in 'us-east-2'." | |
"|NETWORK_CONNECTION") | |
with pytest.raises(OSError, match=msg) as exc: | |
fs.get_file_info("voltrondata-labs-datasets") | |
# Sometimes fails on unrelated network error, so next call would also fail. | |
if 'NETWORK_CONNECTION' in str(exc.value): | |
return | |
fs = S3FileSystem(region='us-east-2', anonymous=True) | |
fs.get_file_info("voltrondata-labs-datasets") |
Maybe you can reuse it? I haven't tested so I am unsure
@raulcd Thanks for looking at it! It turns out For other APIs, I was able to use
So I should be able to add a test for this.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use minio for several tests, see this fixture:
arrow/python/pyarrow/tests/conftest.py
Lines 210 to 245 in 69d2487
def s3_server(s3_connection, tmpdir_factory): | |
@retry(attempts=5, delay=1, backoff=2) | |
def minio_server_health_check(address): | |
resp = urllib.request.urlopen(f"http://{address}/minio/health/live") | |
assert resp.getcode() == 200 | |
tmpdir = tmpdir_factory.getbasetemp() | |
host, port, access_key, secret_key = s3_connection | |
address = f'{host}:{port}' | |
env = os.environ.copy() | |
env.update({ | |
'MINIO_ACCESS_KEY': access_key, | |
'MINIO_SECRET_KEY': secret_key | |
}) | |
args = ['minio', '--compat', 'server', '--quiet', '--address', | |
address, tmpdir] | |
proc = None | |
try: | |
proc = subprocess.Popen(args, env=env) | |
except OSError: | |
pytest.skip('`minio` command cannot be located') | |
else: | |
# Wait for the server to startup before yielding | |
minio_server_health_check(address) | |
yield { | |
'connection': s3_connection, | |
'process': proc, | |
'tempdir': tmpdir | |
} | |
finally: | |
if proc is not None: | |
proc.kill() | |
proc.wait() |
Could we replicate your local validation with a test?
Signed-off-by: Kit Lee <[email protected]>
182cb18
to
7365a2d
Compare
Added some tests. |
Rationale for this change
It is a uuid useful for debugging with AWS support team.
Indeed,
minio
errors will print out a request ID by default.What changes are included in this PR?
The request ID is appended to the end of the error message.
Are these changes tested?
No new test added yet.
I will try to test it on a real S3 system. Minio seems to return empty string only.
Are there any user-facing changes?
No