On Thu, May 7, 2015 at 7:29 AM, gemmellr <g...@git.apache.org> wrote:
> Github user gemmellr commented on a diff in the pull request: > > https://github.com/apache/qpid-proton/pull/29#discussion_r29843571 > > --- Diff: > proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java --- > @@ -157,12 +165,51 @@ public void dispatch(Handler handler) > case TRANSPORT_CLOSED: > handler.onTransportClosed(this); > break; > + case REACTOR_FINAL: > + handler.onReactorFinal(this); > + break; > + case REACTOR_QUIESCED: > + handler.onReactorQuiesced(this); > + break; > + case REACTOR_INIT: > + handler.onReactorInit(this); > + break; > + case SELECTABLE_ERROR: > + handler.onSelectableError(this); > + break; > + case SELECTABLE_EXPIRED: > + handler.onSelectableExpired(this); > + break; > + case SELECTABLE_FINAL: > + handler.onSelectableFinal(this); > + break; > + case SELECTABLE_INIT: > + handler.onSelectableInit(this); > + break; > + case SELECTABLE_READABLE: > + handler.onSelectableReadable(this); > + break; > + case SELECTABLE_UPDATED: > + handler.onSelectableWritable(this); > + break; > + case SELECTABLE_WRITABLE: > + handler.onSelectableWritable(this); > + break; > + case TIMER_TASK: > + handler.onTimerTask(this); > + break; > default: > handler.onUnhandled(this); > break; > } > + > + Iterator<Handler> children = handler.children(); > --- End diff -- > > Commenting here to avoid spamming the semi-unrelated JIRA mentioned > linked with the other PR that got merged. > > I have only skimmed this PR, since I haven't got much of experience of > the reactor code in any of the other languages, so I'm not sure what many > things are actually meant to do. > > It felt a little strange at times that some of the Connection etc > objects now have some very reactor specific methods even though you might > not be using the reactor with them (i.e all existing use), but I can > certainly live with that if thats how it works elsewhere, and they > presumably dont hurt anything if you dont use them. > There may be a way to reduce the coupling a little bit there (I'm currently looking into that), but in the end it simply becomes awkward to write event handlers without being able to navigate from all the various engine objects back to the reactor that holds them. > I did spot this 1 specific bit though, where I think it would be nice > to optimise for what anyone using this currently will be doing. Existing > handlers wont have any children, and that would currently mean that every > time you dispatch an event you will create a pointless iterator, so it > would be nice to avoid that in the case of no children. > Agreed. I need to tweak event dispatch to support some other scenarios, so I'll probably be messing with that code anyways. I'll make sure it doesn't create extraneous objects when I do. --Rafael
