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

job messages list field #3373

Merged
merged 2 commits into from
Sep 24, 2019
Merged

job messages list field #3373

merged 2 commits into from
Sep 24, 2019

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Sep 20, 2019

These changes partially address an the absence of job messages mentioned in cylc/cylc-ui#249

Where Task cycle instance (proxy) messages are being used with every job.. However, as noted:

The latest_message is a field of the TaskProxy not Job, and is updated in much the same way the old state summary did it:

       tproxy.latest_message = itask.summary['latest_message']

However, there will be some mechanism to update the itask.summary['latest_message'] so no reason why we can't add the field to each Job instead...

So on Riot we came to the decision that all messages associated with a job should be held in the job element.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required.
  • No documentation update required.

@dwsutherland dwsutherland added this to the cylc-8.0a2 milestone Sep 20, 2019
@dwsutherland dwsutherland self-assigned this Sep 20, 2019
@dwsutherland
Copy link
Member Author

For example:

query {
  jobs (workflows: ["*|*"], sort: {keys: ["submitNum"]}) {
    id
    submitNum
    state
    messages
  }
}
{
  "data": {
    "jobs": [
      {
        "id": "sutherlander|noz|20190101T00|foo|1",
        "submitNum": 1,
        "state": "failed",
        "messages": [
          "job file written (edit/dry-run)",
          "submitted",
          "WootWoot! I'm running (^.^)",
          "started",
          "Off a cliff! Oh nooooo...",
          "failed/EXIT"
        ]
      },
      {
        "id": "sutherlander|noz|20190101T00|foo|2",
        "submitNum": 2,
        "state": "running",
        "messages": [
          "submitted",
          "started"
        ]
      }
    ]
  }
}

@kinow
Copy link
Member

kinow commented Sep 22, 2019

One failure on Travis

[10:17:07] ./tests/shutdown/06-kill-fail.t .................................. Failed 1/3 subtests 

...


$ .travis/after_failure.sh
==== /home/travis/cylc-run/cylctb-20190920T094618Z/shutdown/06-kill-fail/log/job/1/t1/01/job.err ====
==== /tmp/travis/cylctb-20190920T094618Z/shutdown/06-kill-fail/06-kill-fail-shutdown.stderr ====
ERROR: condition not satisfied after 10 polls
==== /tmp/travis/cylctb-20190920T094618Z/shutdown/06-kill-fail/06-kill-fail-shutdown.stdout ====
polling for 'suite stopped'..........

Just restarted the job that failed.

@dwsutherland
Copy link
Member Author

Gonna add some unit-tests on job_pool.py...

@dwsutherland
Copy link
Member Author

What'sup with Codacy? Can't see any Errors.. it's just not doing the analysis..
https://app.codacy.com/manual/Cylc/cylc-flow/pullRequest?prid=4193032

@matthewrmshin
Copy link
Contributor

Just kicked Codacy...

@cylc cylc deleted a comment Sep 23, 2019
@dwsutherland
Copy link
Member Author

100% coverage 🎉

Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

I trust that you have taken into account my comment about the event time. The 100% test coverage is encouraging.

@kinow
Copy link
Member

kinow commented Sep 24, 2019

Seems to be working as expected

image

@dwsutherland just out of curiosity, is the order of the messages guaranteed be from the oldest to the newest, always? Just asking as I think when we prepare to display it on the UI, we will probably need to provide them in order for the users?

@kinow
Copy link
Member

kinow commented Sep 24, 2019

(or do we need a timestamp next to the message?)

kinow
kinow previously requested changes Sep 24, 2019
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

One issue in the test, and one question (not blocker). Other than that, perfect! 💯 tested with GraphiQL, worked with no issues, and code looking good!

cylc/flow/tests/test_job_pool.py Outdated Show resolved Hide resolved
Add more coverage
@dwsutherland
Copy link
Member Author

dwsutherland commented Sep 24, 2019

I trust that you have taken into account my comment about the event time. The 100% test coverage is encouraging.

Yes, I think stamp needs to be commented on centrally in the data manager and protobuf definition when I do the related PR.

@dwsutherland just out of curiosity, is the order of the messages guaranteed be from the oldest to the newest, always? Just asking as I think when we prepare to display it on the UI, we will probably need to provide them in order for the users?

Yes protobuf repeated fields are ordered lists that can be accessed via index. And I just append new messages (no removal or manipulation).. Then GraphQL resolvers pass the entire job element for query resolving.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Had a look at the last change, to fix the missing import, and it looks good now.

Build passed before, so no need to wait for Travis. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants