On Wed, Jan 18, 2023 at 5:37 PM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > On Wed, Jan 18, 2023 at 1:49 PM wangw.f...@fujitsu.com > <wangw.f...@fujitsu.com> wrote: > > > > On Wed, Jan 18, 2023 at 13:29 PM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > On Tue, Jan 17, 2023 at 6:41 PM Ashutosh Bapat > > > <ashutosh.bapat....@gmail.com> wrote: > > > > > > > > On Tue, Jan 17, 2023 at 3:34 PM Amit Kapila <amit.kapil...@gmail.com> > > > > wrote: > > > > > > > > > > > > > > > > I am a bit worried about the indirections that the wrappers and > > > > > > hooks > > > > > > create. Output plugins call OutputPluginUpdateProgress() in > > > > > > callbacks > > > > > > but I don't see why ReorderBufferProcessTXN() needs a callback to > > > > > > call OutputPluginUpdateProgress. > > > > > > > > > > > > > > > > Yeah, I think we can do it as we are doing the previous approach but > > > > > we need an additional wrapper (update_progress_cb_wrapper()) as the > > > > > current patch has so that we can add error context information. This > > > > > is similar to why we have a wrapper for all other callbacks like > > > > > change_cb_wrapper. > > > > > > > > > > > > > Ultimately OutputPluginUpdateProgress() will be called - which in turn > > > > will call ctx->update_progress. > > > > > > > > > > No, update_progress_cb_wrapper() should directly call > > > ctx->update_progress(). The key reason to have a > > > update_progress_cb_wrapper() is that it allows us to add error context > > > information (see the usage of output_plugin_error_callback). > > > > I think it makes sense. This also avoids the need for every output plugin to > > implement the callback. So I tried to improve the patch based on this > > approach. > > > > And I tried to add some comments for this new callback to distinguish it > > from > > ctx->update_progress. > > Comments don't help when using cscope or some such code browsing tool. > Better to use a different variable name. >
+ /* + * Callback to be called when updating progress during sending data of a + * transaction (and its subtransactions) to the output plugin. + */ + ReorderBufferUpdateProgressCB update_progress; Are you suggesting changing the name of the above variable? If so, how about apply_progress, progress, or updateprogress? If you don't like any of these then feel free to suggest something else. If we change the variable name then accordingly, we need to update ReorderBufferUpdateProgressCB as well. -- With Regards, Amit Kapila.