You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by bozzzzo <gi...@git.apache.org> on 2015/07/29 15:07:17 UTC
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
GitHub user bozzzzo 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/bozzzzo/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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
+ * <code>final static</code> fields. EventType instances are compared by
+ * reference.
+ * <p>
+ * Event types are best described by an <code>enum</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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36174299
--- 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 --
Would an equals() check be more appropriate now that EventType isn't necessarily an Enum (though granted it is natural and we know it is for the core events)?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36186389
--- 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 --
> I'd be more inclined to drop the Reactor events from the Type given that there are currently no users of those events yet (its never been released)
If by drop you mean move reactor events to a future o.a.q.p.reactor.ReactorEvent.Type then
yes, that is exactly what I had in mind for a perfect world. It would be a code-massacre, tho. Deleting them outright would be a mistake, they are useful, and used ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36178745
--- 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 {
+ void onConnectionInit(Event e);
+ void onConnectionLocalOpen(Event e);
+ void onConnectionRemoteOpen(Event e);
+ void onConnectionLocalClose(Event e);
+ void onConnectionRemoteClose(Event e);
+ void onConnectionBound(Event e);
+ void onConnectionUnbound(Event e);
+ void onConnectionFinal(Event e);
+
+ void onSessionInit(Event e);
+ void onSessionLocalOpen(Event e);
+ void onSessionRemoteOpen(Event e);
+ void onSessionLocalClose(Event e);
+ void onSessionRemoteClose(Event e);
+ void onSessionFinal(Event e);
+
+ void onLinkInit(Event e);
+ void onLinkLocalOpen(Event e);
+ void onLinkRemoteOpen(Event e);
+ void onLinkLocalDetach(Event e);
+ void onLinkRemoteDetach(Event e);
+ void onLinkLocalClose(Event e);
+ void onLinkRemoteClose(Event e);
+ void onLinkFlow(Event e);
+ void onLinkFinal(Event e);
+
+ void onDelivery(Event e);
+ void onTransport(Event e);
+ void onTransportError(Event e);
+ void onTransportHeadClosed(Event e);
+ void onTransportTailClosed(Event e);
+ void onTransportClosed(Event e);
+
+ void onReactorInit(Event e);
--- End diff --
Yeah, that's where I want(ed) to go, but it would make for a much more invasive change.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36522483
--- 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 --
May now be wrong rather than definitely is wrong was my point, its now ambiguous due to the change to an interface for the event type...though I do wish I had just never bothered making the comment :)
Context is probably more interesting, in that as it has been used it was to tie a particular proton object to a particular non-proton object for reference, so I can see there may be cases where you actually wouldnt want to de-dup for equivalent contexts and so using equals() instead of == could actually be wrong for the context.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36173420
--- 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 --
NOT_A_BUILTIN_TYPE similarly seems a bit off as a name. Perhaps following in the theme of the work, something like EXTENDED, or following the earlier suggestion, NON_CORE, etc?
Also, for the javadoc link, the Event#getBuiltinType() method doesnt exist.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36178479
--- 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 --
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36179765
--- 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 --
Yep that seems good.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo commented on the pull request:
https://github.com/apache/qpid-proton/pull/48#issuecomment-127776872
@gemmellr thanks for the review! I think I addressed all the things you pointed out.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the pull request:
https://github.com/apache/qpid-proton/pull/48#issuecomment-127622789
> I'm not sure how this would work, because it's easy (and I think necessary) to mix and create a tangle > of handlers of different descent in a tree (or even DAG) of handlers with Handler#add(Handler).
>
> So an Event instance will get dispatched to many handlers, and necessarily only some will be of an assignable type. I'm not sure how to express that with generics.
Yeah, the rest of my reply was getting at that. It just seemed a little icky that everyone who implements a handler should need the instanceof check and call unhandled othewise. Not saying I have an answer :)
> The core can afford the luxury of declaring 'everybody extend me' but that is not viable for a library that sits on top of proton.
Not sure I understood this bit. If I was writing a library on top of Proton I'd possibly want to declare my own 'everybody extend me' point if things outwith that library were expected to extend it (to try and insulate somewhat from changes like this in Proton).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36189359
--- 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 --
Ok. Sounds like you might be using this in a slightly different way than I was thinking, which is likely because I haven't actually used any of the reactor (or handler) stuff at all yet :)
It would be a good idea for some other folks to look this over who actually have, like @rhs and @prestona.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36174204
--- 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 --
Missing braces on if statements.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-proton/pull/48
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] qpid-proton pull request: PROTON-964: Proton-J
extensible event ty...
Posted by Bozo Dragojevic <bo...@digiverse.si>.
If there are no objections to this, I'll commit this tomorrow
Bozzo
On 29. 07. 15 15.07, bozzzzo wrote:
> GitHub user bozzzzo 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/bozzzzo/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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36174093
--- 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 seems to be dispatcher-specific, might be clearer if we named it so?
Same as before regarding the 'Builtin'.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36180258
--- 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 --
Why this needs to be so is more obvious if you consider how Core events (maybe they should be Engine events) would look if reactor had the ReallyReallyCore events :)
Wdyt, replace Core with Engine in all above comments and leave Core for the deeper refactoring.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo commented on the pull request:
https://github.com/apache/qpid-proton/pull/48#issuecomment-127597879
> Could we use generics to enforce the correct type of handler is supplied for dispatch and avoid some instanceof checks? (e.g BuiltinEventTypeImpl.java)
I'm not sure how this would work, because it's easy (and I think necessary) to mix and create a tangle of handlers of different descent in a tree (or even DAG) of handlers with Handler#add(Handler).
So an Event instance will get dispatched to many handlers, and necessarily only some will be of an assignable type. I'm not sure how to express that with generics.
The core can afford the luxury of declaring 'everybody extend me' but that is not viable for a library that sits on top of proton.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36181543
--- 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 --
I'd be more inclined to drop the Reactor events from the Type given that there are currently no users of those events yet (its never been released), whereas the others have been part of the main API for using the engine for quite some time and changing them would definitely hurt a bunch of folks. That said, my comment wasn't really about anything like that, just noting a dislike of needing to do the instanceof but also recognising why :)
I'm not sure about using 'Egnine'. I did actually think of that name earlier and decided agaisnt suggesting it. I'd say the older events are 'more core' in some ways than the new reactor ones, with the reactor being built on top of the eingine...but I guess also has IO related events that dont. Having both 'engine' and 'reactor' events named Core right now seems nicer in some ways than having the reactor events named 'Engine' events. I dunno. Mainly I just disliked 'Builtin' :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 Accessor<T> {
--- 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36174338
--- 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 --
Is there a reason to rename this?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36173162
--- 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 {
+ void onConnectionInit(Event e);
+ void onConnectionLocalOpen(Event e);
+ void onConnectionRemoteOpen(Event e);
+ void onConnectionLocalClose(Event e);
+ void onConnectionRemoteClose(Event e);
+ void onConnectionBound(Event e);
+ void onConnectionUnbound(Event e);
+ void onConnectionFinal(Event e);
+
+ void onSessionInit(Event e);
+ void onSessionLocalOpen(Event e);
+ void onSessionRemoteOpen(Event e);
+ void onSessionLocalClose(Event e);
+ void onSessionRemoteClose(Event e);
+ void onSessionFinal(Event e);
+
+ void onLinkInit(Event e);
+ void onLinkLocalOpen(Event e);
+ void onLinkRemoteOpen(Event e);
+ void onLinkLocalDetach(Event e);
+ void onLinkRemoteDetach(Event e);
+ void onLinkLocalClose(Event e);
+ void onLinkRemoteClose(Event e);
+ void onLinkFlow(Event e);
+ void onLinkFinal(Event e);
+
+ void onDelivery(Event e);
+ void onTransport(Event e);
+ void onTransportError(Event e);
+ void onTransportHeadClosed(Event e);
+ void onTransportTailClosed(Event e);
+ void onTransportClosed(Event e);
+
+ void onReactorInit(Event e);
--- End diff --
I think it could be argued that the reactor-specific events might be better in their own extended handler, but they aren't currently so lets save that for later ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36188742
--- 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 --
'wrong thing' as in fail to coalesce the 'duplicate' events like it is trying to, in the case an EventType instance ever finds its way in there that is not an Enum and isn't used via a single instance, which wouldnt have been possible before but is now, however unlikely.
There are other cases where the code does an == that I didn't say anything about because they are comparing against a specific enum value so their behaviour can't change, but it just seemed like in this case when the line was already being changed anyway it would be simple to avoid any future issues by making the update now.
The comment wasnt meant to be some big thing :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 Accessor<T> {
--- 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/48#discussion_r36188924
--- 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 --
Yes, 'drop from Type' as in move somewhere else, not get rid of entirely as they are obviously used.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by gemmellr <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-964: Proton-J extensible event ty...
Posted by bozzzzo <gi...@git.apache.org>.
Github user bozzzzo 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---