You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/05/01 20:22:22 UTC

[GitHub] [airflow] fuyi opened a new pull request #8675: show correct duration on graph view for running task (#8311)

fuyi opened a new pull request #8675:
URL: https://github.com/apache/airflow/pull/8675


   This PR is aim to fix issue https://github.com/apache/airflow/issues/8311
   
   ---
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [ ] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Target Github ISSUE in description if exists
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [ ] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.


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

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

[GitHub] [airflow] fuyi commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
fuyi commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-635840286


   Hi @ashb , Do you have any comment for this PR?  : )


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

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

[GitHub] [airflow] jeffolsi commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
jeffolsi commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-623073288


   If I may point Invalid date is also misleading. 
   ![PR](https://user-images.githubusercontent.com/64190742/80909263-8a361180-8d2f-11ea-9e4e-1fd41e6ae232.png)
   
   There is nothing invalid here. It just didn't finish so there is no date in Ended.
   It should be also modified to "still running" or "in progress" or something else that won't be considered as problem.


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

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

[GitHub] [airflow] ashb merged pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
ashb merged pull request #8675:
URL: https://github.com/apache/airflow/pull/8675


   


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

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

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-650562577


   Awesome work, congrats on your first merged pull request!
   


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

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

[GitHub] [airflow] fuyi commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
fuyi commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-643977916


   @ashb Do you have more comments? Can I get approval from you : )


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

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

[GitHub] [airflow] fuyi commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
fuyi commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-623418565


   Hi @jeffolsi, Thanks for the comment. I agree with you that "invalid date" is not correct either. I have talked with @XD-DENG and leave this out of the scope of this PR.


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

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

[GitHub] [airflow] fuyi commented on a change in pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
fuyi commented on a change in pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#discussion_r440004656



##########
File path: airflow/www/static/js/task-instances.js
##########
@@ -20,12 +20,18 @@
 /* global window, dagTZ, moment, convertSecsToHumanReadable */
 
 // We don't re-import moment again, otherwise webpack will include it twice in the bundle!
-import { defaultFormat, formatDateTime } from './datetime-utils';
 import { escapeHtml } from './base';
+import { defaultFormat, formatDateTime } from './datetime-utils';
 
 function makeDateTimeHTML(start, end) {
+  // check task ended or not
+  if (end && end instanceof moment) {
+    return (
+      `Started: ${start.format(defaultFormat)} <br> Ended: ${end.format(defaultFormat)} <br>`
+    )
+  }
   return (
-    `Started: ${start.format(defaultFormat)} <br> Ended: ${end.format(defaultFormat)} <br>`
+    `Started: ${start.format(defaultFormat)} <br> Ended: Not ended yet <br>`

Review comment:
       Thanks for the comment @feluelle. However, I think it is more informative to keep `Ended: Not ended yet`




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

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

[GitHub] [airflow] feluelle commented on a change in pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#discussion_r439272790



##########
File path: airflow/www/static/js/task-instances.js
##########
@@ -20,12 +20,18 @@
 /* global window, dagTZ, moment, convertSecsToHumanReadable */
 
 // We don't re-import moment again, otherwise webpack will include it twice in the bundle!
-import { defaultFormat, formatDateTime } from './datetime-utils';
 import { escapeHtml } from './base';
+import { defaultFormat, formatDateTime } from './datetime-utils';
 
 function makeDateTimeHTML(start, end) {
+  // check task ended or not
+  if (end && end instanceof moment) {
+    return (
+      `Started: ${start.format(defaultFormat)} <br> Ended: ${end.format(defaultFormat)} <br>`
+    )
+  }
   return (
-    `Started: ${start.format(defaultFormat)} <br> Ended: ${end.format(defaultFormat)} <br>`
+    `Started: ${start.format(defaultFormat)} <br> Ended: Not ended yet <br>`

Review comment:
       Or maybe don't display `Ended:` at all?
   ```suggestion
       `Started: ${start.format(defaultFormat)} <br>`
   ```




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

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

[GitHub] [airflow] fuyi commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
fuyi commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-650548162


   Thanks for comment @ashb , The screenshot is updated now


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

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

[GitHub] [airflow] feluelle commented on a change in pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#discussion_r440005381



##########
File path: airflow/www/static/js/task-instances.js
##########
@@ -20,12 +20,18 @@
 /* global window, dagTZ, moment, convertSecsToHumanReadable */
 
 // We don't re-import moment again, otherwise webpack will include it twice in the bundle!
-import { defaultFormat, formatDateTime } from './datetime-utils';
 import { escapeHtml } from './base';
+import { defaultFormat, formatDateTime } from './datetime-utils';
 
 function makeDateTimeHTML(start, end) {
+  // check task ended or not
+  if (end && end instanceof moment) {
+    return (
+      `Started: ${start.format(defaultFormat)} <br> Ended: ${end.format(defaultFormat)} <br>`
+    )
+  }
   return (
-    `Started: ${start.format(defaultFormat)} <br> Ended: ${end.format(defaultFormat)} <br>`
+    `Started: ${start.format(defaultFormat)} <br> Ended: Not ended yet <br>`

Review comment:
       I am totally fine with 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.

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

[GitHub] [airflow] fuyi commented on pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
fuyi commented on pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#issuecomment-640220082


   Hi @ashb, `Ended: invalid date` is now fixed 
   
   ![Screenshot 2020-06-07 at 15 28 01](https://user-images.githubusercontent.com/295515/83970133-49a05980-a8d4-11ea-8a7b-e38782b7e162.png)
   


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

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

[GitHub] [airflow] ashb commented on a change in pull request #8675: show correct duration on graph view for running task (#8311)

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #8675:
URL: https://github.com/apache/airflow/pull/8675#discussion_r443262258



##########
File path: airflow/www/static/js/task-instances.js
##########
@@ -80,6 +88,17 @@ export default function tiTooltip(ti, {includeTryNumber = false} = {}) {
   } else {
     tt += `Started: ${escapeHtml(ti.start_date)}<br>`;
   }
+  // Calculate duration on the fly if task instance is still running
+  if(ti.state === "running") {
+    let start_date;
+    if (ti.start_date instanceof moment) {
+      start_date = ti.start_date
+    } else {
+      start_date = moment(ti.start_date)
+    }

Review comment:
       ```suggestion
       let start_date = ti.start_date instanceof moment ? ti.start_date : moment(ti.start_date);
   ```




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

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