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

fix(providers/amazon): handle missing LogUri in emr describe_cluster API response #31482

Merged

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented May 23, 2023

According to the this document, it seems we might not always be able to get ["Cluster"]["LogUri"], and we encounter errors after the release in #31322. In this PR, I set the return value of get_log_uri to None if we cannot get LogUri

Closes: #31480


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@ferruzzi
Copy link
Contributor

If this returns none, what happens with the displayed link that gets generated from this value?

@Lee-W Lee-W force-pushed the handle-missing-loguri-in-emr-get-cluster branch from 171035e to 552bf22 Compare May 24, 2023 03:26
@Lee-W
Copy link
Member Author

Lee-W commented May 24, 2023

@ferruzzi Hi, I just tested it. It will link to a None bucket.

@Lee-W
Copy link
Member Author

Lee-W commented May 24, 2023

Should I disable it if LogUri not found?

@Lee-W
Copy link
Member Author

Lee-W commented May 24, 2023

Hi @potiuk , I encounter the following CI failure. But it doesn't seem to be something I introduce in this PR. Should I fix it in this PR? If so, could you kindly point out where this is from? Thanks!

Lint OpenAPI using openapi-spec-validator..............................................Failed
- hook id: lint-openapi
- exit code: 1

'_io.BufferedReader' object has no attribute 'decode'

@pankajkoti
Copy link
Member

pankajkoti commented May 24, 2023

Hi @potiuk , I encounter the following CI failure. But it doesn't seem to be something I introduce in this PR. Should I fix it in this PR? If so, could you kindly point out where this is from? Thanks!

Lint OpenAPI using openapi-spec-validator..............................................Failed
- hook id: lint-openapi
- exit code: 1

'_io.BufferedReader' object has no attribute 'decode'

@Lee-W I see a similar issue in my PR and have added a comment regarding this #31201 (comment)

Apparently, @uranusjr did try to reproduce this with the PR #31489 as this was observed in main too, but we failed to reproduce it there and main also seemed to run fine later. This is not reproducible locally and looks like some weird issue. I would say we could wait for a while.

@Lee-W Lee-W force-pushed the handle-missing-loguri-in-emr-get-cluster branch from 552bf22 to 0bc3902 Compare May 24, 2023 09:24
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

LGTM

@hussein-awala
Copy link
Member

@ferruzzi Hi, I just tested it. It will link to a None bucket.
Should I disable it if LogUri not found?

IMO this is not a requirement to merge this fix, but it's a nice enhancement for the AWS operators links which can be implemented separately.
@ferruzzi @o-nikolas wdyt?

@ferruzzi
Copy link
Contributor

IMO this is not a requirement to merge this fix, but it's a nice enhancement for the AWS operators links which can be implemented separately.
@ferruzzi @o-nikolas wdyt?

I'm not thrilled with the idea of knowingly vending a broken link. What about a compromise for now where if the link isn't available, return a string along the lines of "N/A"?I Love the idea of disabling the link if it's not available, but a static string message would be better than a dead link IMHO

@pankajkoti
Copy link
Member

pankajkoti commented May 24, 2023

IMO this is not a requirement to merge this fix, but it's a nice enhancement for the AWS operators links which can be implemented separately.
@ferruzzi @o-nikolas wdyt?

I'm not thrilled with the idea of knowingly vending a broken link. What about a compromise for now where if the link isn't available, return a string along the lines of "N/A"?I Love the idea of disabling the link if it's not available, but a static string message would be better than a dead link IMHO

+1 to this idea and perhaps some more description in the string to include something like "you have disabled logging and hence ...." based upon the comment #31480 (comment) might hint the user why it is N/A, no?

@Lee-W
Copy link
Member Author

Lee-W commented May 30, 2023

Hi @ferruzzi @pankajkoti , thanks for your suggestions! I'm not sure how I can add those descriptions but I disabled it if the link is not available.

image

@Lee-W Lee-W force-pushed the handle-missing-loguri-in-emr-get-cluster branch from b316e20 to a0ec623 Compare May 30, 2023 08:30
@ferruzzi
Copy link
Contributor

Hi @ferruzzi @pankajkoti , thanks for your suggestions! I'm not sure how I can add those descriptions but I disabled it if the link is not available.

I love it, thanks for doing that. I think pankajkoti's suggestion was around the phrasing where you put "No URL found for EMR Cluster logs". My initial suggestion was super vague and I think they were suggesting it be more descriptive. I think you have it covered, but I'll leave that to them.

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

I'll approve pending passing CI. I have seem that test fail on occasion for a timeout, try bumping it again.

Copy link
Member

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

Should we add a test here for this? At least one with covering that if cluster_info does not contain LogUri, it does not raise an exception anymore?

Looks good to me otherwise.

@jedcunningham
Copy link
Member

LGTM, but missing test coverage.

@Lee-W Lee-W force-pushed the handle-missing-loguri-in-emr-get-cluster branch from a0ec623 to 4d414b9 Compare May 31, 2023 07:44
@Lee-W
Copy link
Member Author

Lee-W commented May 31, 2023

@pankajkoti Thanks for your suggestion! I just added it.

@hussein-awala hussein-awala added the type:bug-fix Changelog: Bug Fixes label May 31, 2023
@hussein-awala hussein-awala merged commit a8c45b0 into apache:main May 31, 2023
@jedcunningham jedcunningham deleted the handle-missing-loguri-in-emr-get-cluster branch June 1, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing LogUri from emr describe-cluster API when executing EmrCreateJobFlowOperator
6 participants