-
Notifications
You must be signed in to change notification settings - Fork 383
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
[server] Create columns for product details #3382
[server] Create columns for product details #3382
Conversation
nullable=True)) | ||
|
||
try: | ||
product_con = op.get_bind() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution could be if we will set these data after the server starts. That way if we failed to connect to a database during migration or something is not properly set (race condition -> I think it is not possible, because we are changing the number of runs with one query but who knows) we will still be able to set these data to the correct value. What do you think @bruntib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think this is fine during migration, so this data is computed only once. Moreover such errors can happen at server start too.
81113ae
to
ecfe66a
Compare
ecfe66a
to
928484f
Compare
928484f
to
1d12e0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing "currently stored runs" column is intentional, right?
@@ -1189,7 +1191,7 @@ def store(self) -> int: | |||
self.__name, run_lock.locked_at) | |||
|
|||
# Actual store operation begins here. | |||
run_id = self.__add_checker_run( | |||
run_id, update_run = self.__add_checker_run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe __add_or_update_run()
is a better name instead of __add_checker_run()
.
nullable=True)) | ||
|
||
try: | ||
product_con = op.get_bind() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think this is fine during migration, so this data is computed only once. Moreover such errors can happen at server start too.
Create separate columns for products to store the number of runs and the latest storage date instead of connecting and getting these information from each product database.
Rendering too many components in the product list page in a single row can really slow down the rendering. For this reason with this patch we will introduce a client side pagination, so we will not show all the product in one round.
@bruntib What do you mean under removing "currently stored runs". We do not remove any columns from the database with this patch. |
1d12e0b
to
3f1d0a6
Compare
I didn't mean database column but "Run store in progress" column on GUI. |
Yes, I removed it, because it's not an important information from the user perspective and also we are not using it anymore. So we spare some space on the gui and also less data query on the server side. |
Create separate columns for products to store the number of runs and
the latest storage date instead of connecting and getting these
information from each product database.