-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add parameter to /vcf endpoint to force async writes to complete before returning response #75
Changes from 4 commits
cfe6dba
0611fd5
97639d1
a098720
84d1b95
ffa08f8
66c265e
0bc50b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,6 +202,30 @@ def __delitem__(self, name: str) -> None: | |
cur.execute(f"DELETE FROM {self.table_name} WHERE vrs_id = ?;", [name]) # nosec B608 | ||
self.conn.commit() | ||
|
||
def wait_for_writes(self): | ||
"""Return true once any currently pending database modifications have been completed.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be missing something, but I don't think this method returns true |
||
if self.batch_thread is not None: | ||
# short circuit if the queue is empty | ||
with self.batch_thread.cond: | ||
if len(self.batch_thread.pending_batch_list) == 0: | ||
ehclark marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return | ||
|
||
# queue an empty batch | ||
batch = [] | ||
self.batch_thread.queue_batch(batch) | ||
# wait for the batch to be removed from the pending queue | ||
while True: | ||
with self.batch_thread.cond: | ||
if ( | ||
len( | ||
list(filter(lambda x: x is batch, self.batch_thread.pending_batch_list)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double checking that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this should be a strict identity check. The empty batch is essentially a sentinel object and batch objects are not modified as they move through the queue. |
||
) | ||
> 0 | ||
): | ||
ehclark marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.batch_thread.cond.wait() | ||
else: | ||
break | ||
|
||
def close(self): | ||
"""Stop the batch thread and wait for it to complete""" | ||
if self.batch_thread is not None: | ||
|
@@ -420,7 +444,7 @@ def queue_batch(self, batch_insert_values: List[Tuple]): | |
:param batch_insert_values: list of tuples where each tuple consists of (vrs_id, vrs_object) | ||
""" | ||
with self.cond: | ||
if batch_insert_values: | ||
if batch_insert_values is not None: | ||
_logger.info("Queueing batch of %s items", len(batch_insert_values)) | ||
while len(self.pending_batch_list) >= self.max_pending_batches: | ||
_logger.debug("Pending batch queue is full, waiting for space...") | ||
|
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.
I don't think the pass is necessary. Is there an existing issue to do this for postgres backend?
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.
IMO we shouldn't worry about implementing it PG-side until there's a use case/demand for doing something like queueing. I think there's a world in which it could be preferable to retain the PG implementation as a more streamlined read/write option.
That said, it might be good to note in the docstring that this method is just a stub to retain compatibility with the base class, but shouldn't do anything unless we decide to implement queueing logic for the postgres writer.
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.
Then I don't think it should be an abstractmethod in the
_Storage
class and should only exist in the snowflake backendThere 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.
I think the
wait_for_writes()
method should remain on_Storage
because it is called fromanyvar.restapi.main
which does not understand which backend it is using. The REST API method implementations only calls methods on the_Storage
base class. Without it on the base class, calling/vcf
withallow_async_write=no
results in a 500 error if the backend store in Postgres.I cleaned up the comments and removed the
pass
line.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.
@ehclark that makes sense. Thanks