Repository navigation
Status heartbeat - #440
Status heartbeat#440
Conversation
| self._job_status_widget = JobStatusListWidget( | ||
| job_service=job_service, job_controller=job_controller | ||
| ) |
There was a problem hiding this comment.
Including this in the plotting widget (as first tab) is a hack for now, consider it temporary until we have figured out which UI components should be where, etc.
7166c34 to
4eea352
Compare
a91e9e8 to
4479899
Compare
|
Is job status written to Kafka to be visible in Nicos? |
Yes, in principle. They proposed to use this format. |
|
Merging since it is holding up other work. Please review anyway, comments will be addressed on follow-up. |
| error_message: str | None = None | ||
| warning_message: str | None = None | ||
| start_time: int | None = None | ||
| end_time: int | None = None |
There was a problem hiding this comment.
Not for now, but might be useful in the future to report things like memory usage in the job status?
There was a problem hiding this comment.
Not sure, those things maybe be more suited for the Grafana/Graylog integration.
| job_statuses.append(status) | ||
| # Track warnings from job operations, or clear them on success | ||
| if status.has_error and status.error_message is not None: | ||
| self._job_warning_messages[job.job_id] = status.error_message |
There was a problem hiding this comment.
I'm sort of getting confused here as to what is an error and what is a warnings (also in the compute_results below)
If I followed correctly, status here is a JobError. But we're putting it in the warning_messages...
There was a problem hiding this comment.
The name is legacy. Renaming to JobReply and adding comments about the warning/error distinction.
|
|
||
| # Determine current state based on job's location in manager | ||
| if job_id in self._active_jobs: | ||
| if job_id in self._finishing_jobs: |
There was a problem hiding this comment.
We're having to check in various places if the job is in _finishing_jobs, or in _active_jobs. Can we set the type of job on the job object itself? Would this make things easier?
Maybe it was a way to categorize the jobs so that we don't need to scan all the jobs if we are looking only for fnishing jobs?
Do we expect to have thousands of jobs? If there are only a few jobs running, maybe iterating over all jobs is fast enough?
There was a problem hiding this comment.
There should only be a few jobs (tens maybe).
Can we set the type of job on the job object itself? Would this make things easier?
I pondered exactly that when first implementing it: Should a job (have to) know its status? My conclusion then was "no", the job is a simple wrapper, only the manager needs to know that (because nothing the job does depends on knowing whether it is scheduled, active, paused, ...). But maybe it would be simpler after all? Not so sure, but next time this code is touched we can consider it again?
| if status.has_error: | ||
| self._logger.error(self._job_manager.format_job_error(status)) | ||
| for error in job_errors: | ||
| if error.has_error: |
There was a problem hiding this comment.
Maybe related to the comment above about confusion as to what is warning and what is error:
I find it strange that an error potentially does not have an error...
| except Exception as e: | ||
| self._logger.error("Error in job status update callback: %s", e) | ||
|
|
||
| def remove_job(self, job_id: JobId) -> None: |
There was a problem hiding this comment.
Does the order of steps in the function matter? If so, can you add a comment about it?
There was a problem hiding this comment.
It should not matter, since currently JobManager is designed for single-threaded use.
| try: | ||
| self._job_list.remove(widget_panel) | ||
| except ValueError: | ||
| # Panel might not be in the list anymore |
There was a problem hiding this comment.
What happens if Panel is not in the list anymore? Can you describe in which situation this arises?
|
Comments are addressed in #464. |
Address review comments of #440.
4 participants
Add job status heartbeat based on x5f2 messages. Status is used to populate a job status list widget. We also use this to replace the temporary job control widget from an earlier PR.
Fixes #436.
I think
JobControllerwill needs some more iterations, and I am postponing writing tests for that to a follow-up.