Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

B/add synchronous job handler #756

Merged
merged 14 commits into from
Oct 20, 2020
Merged

Conversation

Alexander-Dubrawski
Copy link
Collaborator

@Alexander-Dubrawski Alexander-Dubrawski commented Sep 7, 2020

This PR is the third of four which aims to refactor the handling of Hyrise jobs (it aims to split #664). Hyrise jobs are interacting directly with the Hyrise to get the meta information and managing the Hyrise settings. We have three kinds of jobs:

  1. jobs that need to be continuously in the background (for example: get the system information)
  2. jobs that will be done asynchronously (for example: loading the tables)
  3. jobs that are done synchronously (for example: sending a SQL query over the SQL endpoint)

Both continuous and asynchronous jobs are executed via the apscheduler (https://apscheduler.readthedocs.io/en/stable/). Esspacillay with the asynchronous jobs that's not necessary and increases the complexity (we could just use normal python threads). All three kinds of jobs are scattered in the database and background_scheduler component. The goal of the refactoring is now to have a clear separation for the jobs in the three categories (continuous, asynchronous, and synchronous) and define handlers for that. Moreover, we can stop the continuous jobs while loading/deleting tables (hyrise/hyrise#2180).

This PR implements the handler for the synchronous jobs.

@Alexander-Dubrawski Alexander-Dubrawski marked this pull request as ready for review October 7, 2020 09:03
Copy link
Contributor

@Bensk1 Bensk1 left a comment

Choose a reason for hiding this comment

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

First round of comments, more tomorrow.

return SqlResultInterface(
id=database_id,
successful=True,
results=[[str(col) for col in row] for row in cur.fetchall()],
results=[[str(col) for col in row] for row in results],
Copy link
Contributor

Choose a reason for hiding this comment

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

Still, why do we need the cast to str here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Inside the flask app, we are serializing and deserializing entities. We are doing that to make the code more robust. For example, if something changes in the response from the manager (database_manager/manager.py) we get an error. Inside the IPC communication, we are using the JSON form for the messages. So that's why we are serializing and deserializing entities. In the case of a SQL result, we expect it to be a string (because the frontend expects the same). That's why we cast it. The python entities are defined by a model in this case it is defined in:

https://github.com/hyrise/Cockpit/blob/dev/hyrisecockpit/api/app/sql/model.py

Inside the PR description of #775 I tried to describe how the serializing and deserializing is handled in our flask app.

@Alexander-Dubrawski Alexander-Dubrawski merged commit d1dfa25 into dev Oct 20, 2020
@Alexander-Dubrawski Alexander-Dubrawski deleted the B/Add_synchronous_job_handler branch October 20, 2020 08:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants