schema refinement, dbjobs speed increase, and added migration routine #922
Conversation
….. and also restore a method that got deleted??
…nd of regular acquisition method, removed helper to run dbjobs as a standalone process
they come from a pending changeset to incorporate (into this branch, 'v4-srrpp-migrations') improvements made in a parallel 'mergedkey' branch... i fear i may just be kicking more merge conflicts down the road, but hopefully this helps me to be less confused about some current diffs in the meantime. changes are: - signal_latest table definition simplified to automatically inherit structure from signal_history table - removal of unused row-counting methods - docstring update / correction
| time_q.append(time.time()) | ||
| logger.debug('signal_load_delete_processed', rows=self._cursor.rowcount, elapsed=time_q[-1]-time_q[-2]) | ||
| except Exception as e: | ||
| raise e |
There was a problem hiding this comment.
Should this exception be added to the logger so it's in the log file?
There was a problem hiding this comment.
Well...
This method is only really called by insert_or_update_batch which in turn will again catch and re-raise this exception, and it states "rollback is handled in csv_to_database; if you're calling this yourself, handle your own rollback".
Also, instantiating the structured_logger registers a handler by default that should log any uncaught exceptions.
Looking at this now (unless im missing something), its really just a no-op to catch a nonspecific exception and just simply re-raise it. The same situation effectively exists in the above mentioned insert_or_update_batch, but its presence coupled with the comment hints to a maintainer that exception handling is not an afterthought and is being dealt with elsewhere.
And to be fair, i cribbed the pattern in this method from @krivard 's version here (because im currently in the middle of incorporating lots of her testing fixes from that branch into this one). She probably had a good reason for this that got lost along the way; perhaps other code was removed, like an Exception subclass case that was handled differently, or a finally clause.
SO i think the correct thing to do here is to remove these no-op catch+reraise block boundaries, BUT i am going to leave them for now because they are harmless and we can discuss their efficacy later (or preferably sooner!). I made a note to address this that i will act on in the very near future, either by fixing it or turning it into a proper github issue.
im not sure why i wrote such a novel for this. ¯\_(ツ)_/¯
There was a problem hiding this comment.
i think the correct thing to do here is to remove these no-op catch+reraise block boundaries, BUT i am going to leave them for now because they are harmless and we can discuss their efficacy later (or preferably sooner!)
+1, agree on both counts
|
Whoops, sorry if i assaulted anyones inboxes with edits i made to comments above!!! I forgot that i was responding to individual review comments and not submitting a single unified review myself (i thought i had to hit submit for them to be sent all at once). |
|
This changeset slightly outgrew the scope of what this branch was supposed to be, so i want to summarize the totality of the delta up to this point:
|
This is the branch with the "constraintz" variant of v4.