On Tuesday, October 18, 2022 10:36 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Hi, here are my review comments for patch v38-0001.
Thanks for the comments. > ~~~ > > 12. get_transaction_apply_action > > I still felt like there should be some tablesync checks/comments in > this function, just for sanity, even if it works as-is now. > > For example, are you saying ([3] #22b) that there might be rare cases > where a Tablesync would call to parallel_apply_find_worker? That seems > strange, given that "for streaming transactions that are being applied > in the parallel ... we disallow applying changes on a table that is > not in the READY state". > > ------ I think because we won't try to start parallel apply worker in table sync worker(see the check in parallel_apply_can_start()), so we won't find any worker in parallel_apply_find_worker() which means get_transaction_apply_action will return TRANS_LEADER_SERIALIZE. And get_transaction_apply_action is a function which can be invoked for all kinds of workers(same is true for all apply_handle_xxx functions), so not sure if table sync check/comment is necessary. Best regards, Hou zj
