Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use addEventListener(EventListener listener) instead of bespoke listener methods #3966

Merged
merged 35 commits into from
Nov 13, 2019
Merged

Use addEventListener(EventListener listener) instead of bespoke listener methods #3966

merged 35 commits into from
Nov 13, 2019

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Aug 13, 2019

For #2578 (includes #3965 for #3964):

  • Use addEventListener rather than bespoke listener methods.
  • Support getEventListenerBeans at Container level for fast lookup
  • improve javadoc

gregw added 5 commits August 13, 2019 16:35
@gregw
Avoid creating listener list for rarely used requestAttributeListener
babfa3d 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #3964
8514201 
Keep a list of EventListeners in the AbstractConnector to make it
more efficient to add and iterate over them.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
565298b 
Use addEventListener rather than bespoke listener methods.
Support getEventListenerBeans at Container level for fast lookup
improve javadoc

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
40cc7bf 
fixed test
more javadoc

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
548af98 
fixed tests

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@joakime
Copy link
Contributor

joakime commented Aug 13, 2019

I don't like this.
The code is needlessly verbose, and a pain to maintain in the future.
Do we have any jmh proof this is even needed?

@sbordet
Copy link
Contributor

sbordet commented Aug 13, 2019

Same as @joakime.

@gregw gregw mentioned this pull request Aug 13, 2019
Merged
gregw added 6 commits August 14, 2019 08:09
@gregw
Issue #3964
06afe72 
Don't use null for empty lists of listeners

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
fix merge
03c0144 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
160ee44 
Resolve differences between eventListeners added as beans and beans
added as EventListeners.   The behaviour should now be the same
regardless of how they listener is added and all listeners are now
beans.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
633889a 
Add only SelectorManager listeners to manager from connector

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Merge branch 'jetty-10.0.x' into jetty-10.0.x-2578-EventListener
216166f 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
058b45a 
Fixed javadoc

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw marked this pull request as ready for review August 14, 2019 03:50
@gregw
Issue #2578 EventListener
d64d6fc 
removed old TODO

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Aug 14, 2019 •
edited
Loading

@joakime @sbordet come on! you've got to give better reviews than "I don't like it"!

What code verbose? I have more lines, but mostly because I've actually added some javadoc?

How is this more difficult to maintain? Previously we have had a managery of different listeners, some EventListeners, some not, some are beans, some are not, some are copied between specific beans and some are not. It's a confusing and already difficult to maintain. Now all EventListeners are beans and all beans that are EventListeners are added as such. We do have some classes called Listener that are not EventListeners, but they probably should have been called Handlers. Now any listener that is-a EventListener can be added with addEventListener rather than sometimes with a bespoke adder.

Why is it "needless"? This clean up avoids several needless iteration over beans and ArrayList creations on every new connection and new HttpChannel (which can be every request in http2). So it is a non-trivial saving of effort.

@gregw
Issue #2578 EventListener
ff607fc 
connector cannot be null

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw added the wip label Aug 14, 2019
@gregw
Copy link
Contributor Author

gregw commented Aug 14, 2019

Doh! I have found an issue with this approach - MBeanListener, which is on every bean if JMX is enabled makes this a non optimal approach. Consider this a draft again until I find a better way.

gregw added 4 commits August 14, 2019 15:16
@gregw
Issue #2578 EventListener
1596f53 
javadoc

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #3964
a4a466f 
AbstractConnector keeps a specific list of HttpChannel.Listeners
to avoid Connection.Listeners and MBean listeners being added to
the HttpChannel listener list.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #2578 EventListener
9ea7d4c 
fixed merge

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
fixed javadoc
1ef4ba0 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw removed the wip label Aug 15, 2019
gregw added 4 commits August 19, 2019 12:32
@gregw
Issue #2578 EventListener
35aef6b 
removed the ability to set/clear context listeners
Instead just remove non-durable ones.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Issue #3964 Listeners
cc72473 
Simplified listener handling by avoiding null connector, previously
only needed for testing.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
fixed bad merge
fa24361 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Merge branch 'jetty-10.0.x' into jetty-10.0.x-2578-EventListener
fc61c47 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added 2 commits August 21, 2019 14:45
@gregw
removed deprecated cyclic HttpChannel listeners
5695dc9 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
removed deprecated cyclic HttpChannel listeners - import
536728e 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@joakime joakime changed the title Jetty 10.0.x #2578 event listener Issue #2578 - Listener behavior cleanup (Jetty 10.0.x) Aug 21, 2019
@joakime joakime added this to the 10.0.x milestone Aug 21, 2019
gregw added 4 commits August 25, 2019 17:36
@gregw
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
8d502e5 
…2578-EventListener

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Merge branch 'jetty-10.0.x' into jetty-10.0.x-2578-EventListener
5a16d6b 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Merge branch 'jetty-10.0.x' into jetty-10.0.x-2578-EventListener
31b772a 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
8498bb5 
…2578-EventListener
@gregw
Copy link
Contributor Author

gregw commented Aug 28, 2019

@janbartel @joakime re-review please

@gregw
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
3a21969 
…2578-EventListener

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested review from sbordet and joakime September 4, 2019 22:36
@gregw
Issue #4003 Cleanup quickstart
abd7629 
* Fixed tests that scan for "Started" on console

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from janbartel September 10, 2019 23:53
@gregw
Merge branch 'jetty-10.0.x' into jetty-10.0.x-2578-EventListener
057f1a3 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
updates from review
946151c 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from janbartel September 24, 2019 03:06
@joakime joakime removed their request for review September 25, 2019 21:30
@gregw gregw merged commit 46a3368 into jetty-10.0.x Nov 13, 2019
@joakime joakime deleted the jetty-10.0.x-2578-EventListener branch January 10, 2020 00:40
@joakime joakime mentioned this pull request Sep 14, 2020
Closed
@joakime joakime changed the title Issue #2578 - Listener behavior cleanup (Jetty 10.0.x) Use addEventListener(EventListener listener) instead of bespoke listener methods Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janbartel janbartel janbartel approved these changes

@sbordet sbordet Awaiting requested review from sbordet

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
10.0.x
Development

Successfully merging this pull request may close these issues.
4 participants
@gregw @joakime @sbordet @janbartel