[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36521660
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/EventType.java ---
@@ -0,0 +1,42 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.proton.engine;
+
+/**
+ * Entry point for external libraries to add event types. Event types
should be
+ * codefinal static/code fields. EventType instances are compared by
+ * reference.
+ * p
+ * Event types are best described by an codeenum/code that implements
the
+ * {@link EventType} interface, see {@link Event.Type}.
+ *
+ * @author bozzo
--- End diff --
Can you drop all the @author tags, we dont tend to use them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36522686
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/CollectorImpl.java ---
@@ -57,9 +58,16 @@ public void pop()
}
}
-public EventImpl put(Event.Type type, Object context)
+public EventImpl put(EventType type, Object context)
{
-if (tail != null tail.getType() == type
+if (type == null) {
+throw new IllegalArgumentException(Type cannot be null);
+}
+if (!type.isValid()) {
+throw new IllegalArgumentException(Cannot put events of type
+ type);
+}
+// XXX: @gemmellr points out that type equality is wrong for event
de-duplication. What about context equality?
--- End diff --
I can respin or drop the commit :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on the pull request: https://github.com/apache/qpid-proton/pull/48#issuecomment-128714379 Hi Bozo, sorry for the delay getting back to you, I was a bit tunnel-visioned trying (and failing :/) to get 0.10 out the door this week. I like the recent change giving the handler a method to handle events. I'm not sure it really reduces (so much as perhaps moves) the casting much, but it at least gives the Handler interface something to do other than onUnhandled and also gives the Handler the decision over how to handle an Event rather than the other way around, which seems nicer. It would still be good for some other folks who actually wrote the event/reactor stuff to take a look (like @rhs and @prestona, poke) but master is open for 0.11 at this point --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36523726
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/CollectorImpl.java ---
@@ -57,9 +58,16 @@ public void pop()
}
}
-public EventImpl put(Event.Type type, Object context)
+public EventImpl put(EventType type, Object context)
{
-if (tail != null tail.getType() == type
+if (type == null) {
+throw new IllegalArgumentException(Type cannot be null);
+}
+if (!type.isValid()) {
+throw new IllegalArgumentException(Cannot put events of type
+ type);
+}
+// XXX: @gemmellr points out that type equality is wrong for event
de-duplication. What about context equality?
--- End diff --
Drop it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on the pull request: https://github.com/apache/qpid-proton/pull/48#issuecomment-128358421 @gemmellr I think I have managed to eliminate almost all casts by delegating half of the dispatch to the handler -- the handler already knows it's type, no need for generics even. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/48#discussion_r36177254 --- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/BuiltinEventTypeImpl.java --- @@ -0,0 +1,144 @@ +package org.apache.qpid.proton.engine.impl; --- End diff -- Missing a license header. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/48#discussion_r36177270 --- Diff: proton-j/src/test/java/org/apache/qpid/proton/engine/EventExtensibilityTest.java --- @@ -0,0 +1,381 @@ +package org.apache.qpid.proton.engine; --- End diff -- Missing a license header. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179723
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/Event.java
---
@@ -80,15 +83,46 @@
SELECTABLE_WRITABLE,
SELECTABLE_EXPIRED,
SELECTABLE_ERROR,
-SELECTABLE_FINAL
+SELECTABLE_FINAL,
+
+/**
+ * This value must never be used to generate an event, it's only
used as
+ * a guard when casting custom EventTypes to builtin {@link Type}
via
+ * {@link Event#getBuiltinType()}.
+ */
+NOT_A_BUILTIN_TYPE;
--- End diff --
That would also work yep.
(I dislike needing to have it at all, but I can see why its there)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/48#discussion_r36181146 --- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java --- @@ -217,6 +101,19 @@ public void dispatch(Handler handler) dispatch(children.next()); } } + +@Override +public void redispatch(EventType as_type, Handler handler) throws HandlerException --- End diff -- This is for now the only way to actually generate non-core, events... I'll extend the javadoc to make it explicit. This was the minimal change needed and right now behaves as you describe. A typical example would be a message decoder handler that turns a DELIVERY event into a MESSAGE event with attached message in the event. Or a timer for periodic sampling of a Connection, and so on. I'm in fact sitting on some more reactor/collector changes that were done later and for other use cases. One of them is the ability for a handler to have children process the event synchronously. I can also fold the implementation of that change in here as it would then allow redispatch() to have child handlers be invoked first with the original event, before redispatching the new type. And keep exposing the interface for that in a separate jira. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36182863
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/CollectorImpl.java ---
@@ -57,9 +58,13 @@ public void pop()
}
}
-public EventImpl put(Event.Type type, Object context)
+public EventImpl put(EventType type, Object context)
{
-if (tail != null tail.getType() == type
+if (type == null)
+throw new IllegalArgumentException(Type cannot be null);
+if (!type.isValid())
+throw new IllegalArgumentException(Cannot put events of type
+ type);
+if (tail != null tail.getEventType() == type
--- End diff --
Can you please qualify 'the wrong thing', I really want to understand this?
With this overall change, I'm only working around the `final` of the enum,
not around other aspects of it's semantics. This change brings us about on-par
with proton-c imho where EventType === int.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36180019
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java ---
@@ -53,9 +54,9 @@
this.type = null;
}
-void init(Event.Type type, Object context)
+void init(EventType type2, Object context)
{
-this.type = type;
+this.type = type2;
--- End diff --
uhuh, will fix
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36180425
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/CollectorImpl.java ---
@@ -57,9 +58,13 @@ public void pop()
}
}
-public EventImpl put(Event.Type type, Object context)
+public EventImpl put(EventType type, Object context)
{
-if (tail != null tail.getType() == type
+if (type == null)
+throw new IllegalArgumentException(Type cannot be null);
+if (!type.isValid())
+throw new IllegalArgumentException(Cannot put events of type
+ type);
+if (tail != null tail.getEventType() == type
--- End diff --
I believe it is coaslescing 'duplicate' events.
Your explanation of the changes you did make here reinforces why I think it
should probably be changed to equals() now. Before your changes, it wasnt
possible for the == check here to do the wrong thing. Following these changes
in future it will be possible (even if not that likely) for this to do the
wrong thing and it wont necessarily be the easiest thing to find then, whereas
it is just now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36173790
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/Record.java
---
@@ -29,6 +29,10 @@
public interface Record
{
+public interface AccessorT {
--- End diff --
Can this be in its own file, e.g 'RecordAccessor'?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36178995
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/Event.java
---
@@ -80,15 +83,46 @@
SELECTABLE_WRITABLE,
SELECTABLE_EXPIRED,
SELECTABLE_ERROR,
-SELECTABLE_FINAL
+SELECTABLE_FINAL,
+
+/**
+ * This value must never be used to generate an event, it's only
used as
+ * a guard when casting custom EventTypes to builtin {@link Type}
via
+ * {@link Event#getBuiltinType()}.
+ */
+NOT_A_BUILTIN_TYPE;
--- End diff --
I'll go with something on the theme NON_CORE_EVENT?
The getBuiltinType() is from a previous iteration of the patch and should
have been renamed to getType()
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179005
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/Record.java
---
@@ -29,6 +29,10 @@
public interface Record
{
+public interface AccessorT {
--- End diff --
Ack
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179169
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/BuiltinEventTypeImpl.java
---
@@ -0,0 +1,144 @@
+package org.apache.qpid.proton.engine.impl;
+
+import org.apache.qpid.proton.engine.BuiltinHandler;
+import org.apache.qpid.proton.engine.Event;
+import org.apache.qpid.proton.engine.Handler;
+
+public class BuiltinEventTypeImpl {
--- End diff --
This is out-of-line implementation of EventType interface for
Event.Type#dispatch.
CoreEventDispatcher?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179236
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/CollectorImpl.java ---
@@ -57,9 +58,13 @@ public void pop()
}
}
-public EventImpl put(Event.Type type, Object context)
+public EventImpl put(EventType type, Object context)
{
-if (tail != null tail.getType() == type
+if (type == null)
--- End diff --
ack
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179361
--- Diff:
proton-j/src/test/java/org/apache/qpid/proton/engine/EventExtensibilityTest.java
---
@@ -0,0 +1,381 @@
+package org.apache.qpid.proton.engine;
+
+import java.io.IOException;
+
+import org.apache.qpid.proton.engine.Event.Type;
+import org.apache.qpid.proton.reactor.Reactor;
+import org.apache.qpid.proton.reactor.Selectable;
+import org.apache.qpid.proton.reactor.Task;
+import org.junit.Test;
+
+import junit.framework.TestCase;
+
+public class EventExtensibilityTest extends TestCase {
+
+// //
+// / Extra package public API classes
+// //
+
+/**
+ * Sample additional information that gets generated and passed around
with
+ * the extension events. The information is stored inside
+ * {@link Event#attachments()} see {@link
ExtraEventImpl#getExtraInfo()}
+ */
+public static class ExtraInfo {
+public int foo;
+}
+
+/**
+ * Additional events generated by this extension.
+ *
+ * @author bozzo
+ *
+ */
+public interface ExtraEvent extends Event {
+/**
+ * Enum is a convenient mechanism for defining new event types
+ */
+public enum ExtraTypes implements EventType {
+FOO, BAR, NOT_A_EXTRA_EVENT;
+;
+
+/**
+ * The actual implementation of the dispatch can be moved out
of
+ * line
+ */
+@Override
+public void dispatch(Event e, Handler h) {
+ExtraEventTypeImpl.dispatch(this, e, h);
+}
+
+@Override
+public boolean isValid() {
+return this != NOT_A_EXTRA_EVENT;
+}
+}
+
+/**
+ * typesafe access to event type generated by this extension
useful for
+ * handling in switch statements
+ *
+ * @return one of enum values. When invoked on an Event that is
not an
+ * ExtraEvent the return value shall be
+ */
+ExtraTypes getExtraEventType();
+
+/**
+ * typesafe access to extra information attached to additional
events
+ */
+ExtraInfo getExtraInfo();
+}
+
+/**
+ * New handler methods for handling the extended event types. These
methods
+ * can take {@link ExtraEvent} as a parameter to get typesafe access to
+ * {@link ExtraInfo}
+ *
+ * The interface needs to extend the {@link Handler} interface.
+ */
+public interface ExtraHandler extends Handler {
+void onFoo(ExtraEvent e);
+
+void onBar(ExtraEvent e);
+}
+
+/**
+ * Implementation of the default base class for ExtraHandler. All
events
+ * should be forwarded to the {@link Handler#onUnhandled(Event)} method
+ */
+public static class ExtraBaseHandler extends BaseHandler implements
ExtraHandler {
+
+@Override
+public void onFoo(ExtraEvent e) {
+this.onUnhandled(e);
+}
+
+@Override
+public void onBar(ExtraEvent e) {
+this.onUnhandled(e);
+}
+
+}
+
+// //
+// / Extra package implementation classes
+// //
+
+public static class ExtraEventTypeImpl {
+/**
+ * Implementation of dispatch method for this extension
+ *
+ * @param type
+ *The implementation should accept only it's own
+ *{@link EventType} instances
+ * @param handler
--- End diff --
Forgot to comment earlier. The javadoc params dont match the method
signature.
(I know, only a test, but we have more than enough javadoc warnings/errors
already :P)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36173016
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/BuiltinHandler.java ---
@@ -0,0 +1,71 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.proton.engine;
+
+public interface BuiltinHandler extends Handler {
--- End diff --
'Builtin' seems a bit off as a name. Perhaps 'Core' / 'CoreEvents' or
something else along those lines?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36174376
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java ---
@@ -68,12 +69,19 @@ void clear()
}
@Override
-public Type getType()
+public EventType getEventType()
{
return type;
}
@Override
+public Type getType() {
+if (type instanceof Type)
--- End diff --
Braces on if statement.
It would be nice if we didnt need to do an instanceof check, though I can
only see making all non-core events implement getType() fixing that... :/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/48#discussion_r36174549 --- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/EventImpl.java --- @@ -217,6 +101,19 @@ public void dispatch(Handler handler) dispatch(children.next()); } } + +@Override +public void redispatch(EventType as_type, Handler handler) throws HandlerException --- End diff -- A little unclear why this is needed, but ignoring that...it seems like using it inside a handler (as in the test) would lead to 'redispatch' to the current and child handlers before the original event is then subsequently given to any child handlers. Expected? Worth documenting? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user gemmellr commented on the pull request: https://github.com/apache/qpid-proton/pull/48#issuecomment-127559169 I've commented on the code, but one final random thought: Could we use generics to enforce the correct type of handler is supplied for dispatch and avoid some instanceof checks? (e.g BuiltinEventTypeImpl.java) Its possibly more trouble than its worth, but it feels a little weird we dispatch to/with only 'Handler' now when it no longer has any methods on it of use in dispatching except onUnhandled(). Given the way all the children for a handler also get called that might not be particularly doable, unless the children are similarly forced to be of compatible types. Of course, that whole process also suggests that onUnhandled is usually going to have to be a no-op anyway, or some way would be needed to filter only particular events to particular children. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Github user boo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179971
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/engine/impl/CollectorImpl.java ---
@@ -57,9 +58,13 @@ public void pop()
}
}
-public EventImpl put(Event.Type type, Object context)
+public EventImpl put(EventType type, Object context)
{
-if (tail != null tail.getType() == type
+if (type == null)
+throw new IllegalArgumentException(Type cannot be null);
+if (!type.isValid())
+throw new IllegalArgumentException(Cannot put events of type
+ type);
+if (tail != null tail.getEventType() == type
--- End diff --
Frankly, I'm not even sure why this check is needed at all.
But extensibe events don't need to change collector semantics. the change
here is
a mostly about robustness and a bit about the fact that not only core
events are now possible.
If needed this should be a separate change with separate justification.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
GitHub user boo opened a pull request: https://github.com/apache/qpid-proton/pull/48 PROTON-964: Proton-J extensible event types You can merge this pull request into a Git repository by running: $ git pull https://github.com/boo/qpid-proton proton-964 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/qpid-proton/pull/48.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #48 commit a72c23a136dddae7efd6385d3b32c406c861a016 Author: Bozo Dragojevic bo...@digiverse.si Date: 2015-07-13T10:57:13Z PROTON-964: Proton-J extensible event types --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
