You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Lee-W (via GitHub)" <gi...@apache.org> on 2023/05/23 15:05:29 UTC

[GitHub] [airflow] Lee-W opened a new pull request, #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Lee-W opened a new pull request, #31482:
URL: https://github.com/apache/airflow/pull/31482

   #31480
   
   <!--
   Thank you for contributing! Please make sure that your code changes
   are covered with tests. And in case of new features or big changes
   remember to adjust the documentation.
   
   Feel free to ping committers for the review!
   
   In case of an existing issue, reference it using one of the following:
   
   closes: #ISSUE
   related: #ISSUE
   
   How to write a good git commit message:
   http://chris.beams.io/posts/git-commit/
   -->
   
   ---
   **^ Add meaningful description above**
   
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
   In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   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](https://github.com/apache/airflow/tree/main/newsfragments).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] hussein-awala merged pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #31482:
URL: https://github.com/apache/airflow/pull/31482


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Lee-W commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1560413452

   Should I disable it if LogUri not found?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] hussein-awala commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1560875637

   > @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?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] hussein-awala commented on a diff in pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #31482:
URL: https://github.com/apache/airflow/pull/31482#discussion_r1203753961


##########
airflow/providers/amazon/aws/links/emr.py:
##########
@@ -54,9 +54,15 @@ def get_log_uri(
             "Requires either the output of a describe_cluster call or both an EMR Client and a job_flow_id."
         )
 
+    log_uri = None
+    parsed_log_uri = None
     if cluster:
-        log_uri = S3Hook.parse_s3_url(cluster["Cluster"]["LogUri"])
+        parsed_log_uri = S3Hook.parse_s3_url(cluster["Cluster"]["LogUri"])
     else:
         response = emr_client.describe_cluster(ClusterId=job_flow_id)
-        log_uri = S3Hook.parse_s3_url(response["Cluster"]["LogUri"])
-    return "/".join(log_uri)
+        if "LogUri" in response["Cluster"]:
+            parsed_log_uri = S3Hook.parse_s3_url(response["Cluster"]["LogUri"])
+
+    if parsed_log_uri:
+        log_uri = "/".join(parsed_log_uri)
+    return log_uri

Review Comment:
   When cluster is provided, we get it by the same method but we provide it to avoid querying the API twice, so we can have the same exception.
   We can simplify this method by using this code:
   ```suggestion
       cluster_info = (cluster or emr_client.describe_cluster(ClusterId=job_flow_id))["Cluster"]
       if "LogUri" not in cluster_info:
           return
       log_uri = S3Hook.parse_s3_url(cluster_info["LogUri"])
       return "/".join(log_uri)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ferruzzi commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1561513582

   > 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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Lee-W commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1560413312

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ferruzzi commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1559905644

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] pankajkoti commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1561849820

   > > 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 "that you have disabled logging and hence ...." based upon the comment https://github.com/apache/airflow/issues/31480#issuecomment-1560723485 might hint the user why it is N/A, no?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Lee-W commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1567996773

   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 avaiable.
   
   ![image](https://github.com/apache/airflow/assets/5144808/c51fe28b-d73c-4725-8f12-42535ed840ec)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ferruzzi commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1568758397

   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jedcunningham commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1569326552

   LGTM, but missing test coverage.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Lee-W commented on a diff in pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #31482:
URL: https://github.com/apache/airflow/pull/31482#discussion_r1203786978


##########
airflow/providers/amazon/aws/links/emr.py:
##########
@@ -54,9 +54,15 @@ def get_log_uri(
             "Requires either the output of a describe_cluster call or both an EMR Client and a job_flow_id."
         )
 
+    log_uri = None
+    parsed_log_uri = None
     if cluster:
-        log_uri = S3Hook.parse_s3_url(cluster["Cluster"]["LogUri"])
+        parsed_log_uri = S3Hook.parse_s3_url(cluster["Cluster"]["LogUri"])
     else:
         response = emr_client.describe_cluster(ClusterId=job_flow_id)
-        log_uri = S3Hook.parse_s3_url(response["Cluster"]["LogUri"])
-    return "/".join(log_uri)
+        if "LogUri" in response["Cluster"]:
+            parsed_log_uri = S3Hook.parse_s3_url(response["Cluster"]["LogUri"])
+
+    if parsed_log_uri:
+        log_uri = "/".join(parsed_log_uri)
+    return log_uri

Review Comment:
   Thanks for your suggestion! I just updated it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Lee-W commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1569668843

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Lee-W commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1560424625

   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'
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] pankajkoti commented on pull request #31482: fix(providers/amazon): handle missing LogUri in emr describe_cluster API response

Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on PR #31482:
URL: https://github.com/apache/airflow/pull/31482#issuecomment-1560647002

   > 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'
   > ```
   
   I see a similar issue in my PR and have added a comment regarding this https://github.com/apache/airflow/pull/31201#issuecomment-1559977621
   
   Apparently, @uranusjr did try to reproduce this with the PR https://github.com/apache/airflow/pull/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org