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. -- Best Wishes, Ashutosh Bapat
