[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
Github user tabish121 commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/20#discussion_r234811453
--- Diff:
proton-j/src/main/java/org/apache/qpid/proton/codec/CompositeReadableBuffer.java
---
@@ -834,22 +834,39 @@ public boolean equals(Object other) {
return false;
}
-ReadableBuffer buffer = (ReadableBuffer)other;
-if (this.remaining() != buffer.remaining()) {
+ReadableBuffer buffer = (ReadableBuffer) other;
+final int remaining = remaining();
+if (remaining != buffer.remaining()) {
return false;
}
+if (hasArray()) {
+return equals(currentArray, position, remaining, buffer);
--- End diff --
For the single array case, when you have a slice that you've created from a
CRB that is not starting from zero then the current offset will be non-zero but
position would be so the equals check should be starting at current offset and
moving forward however many elements the limit allows for.
The position and the offset work together to control where zero is in the
buffer, you can do a get for some index in the buffer and the position controls
how far back you can rewind the offset until you've hit "zero" which might not
be zero in the array.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton-j/pull/20 @franz1981 Looks good so far, would like to see the formatting changed to match the remainder of the code. The reset is a good catch, that is a bug in the current code. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #26: QPIDJMS-430 Lock-Free FifoMessageQueue
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/26 Yes, that test is exposing some issues in the 5.x broker which is unrelated to this change. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #23: onConnectionEstablished(): log only main broker URI part...
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/23 Possible race in the test, seems unrelated --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #23: onConnectionEstablished(): log only main broker URI part...
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/23 You should open a new JIRA to track this and use the JIRA entry in the commit title in order to link the two. Refer to the commits in the log already for a proper example. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #19: Save allocation of new promise on each writeAndFlush
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/19 I spent some time reviewing netty code and getting a handle on how this would change the current behavior. From what I can see this should be safe to apply. The tests all pass from a local checkout as well. I'd recommend creating a JIRA and updating the PR with the JIRA issue to link up the work done here. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #17: QPIDJMS-374: Use getRawQuery to avoid double encoding
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/17 You need to squash the commits and force push so that it is contained in one commit, thanks. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #17: QPIDJMS-374: Use getRawQuery to avoid double encoding
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/17 I believe the correct fix is to call PropertyUtil.parseQuery(remoteURI) instead and it will do the right thing, although we can't know until you provide some tests to validate it. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #12: QPIDJMS-315: Add support for Netty KQueue transport
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/12 @michaelandrepearce The Epoll warning has been reported to Netty a few days ago, see [Issue:7120](https://github.com/netty/netty/issues/7120) which was reported by someone using it on ARM. You might want to add onto that one your experiences with Epoll on Artemis and also with KQueue, you could even provide a PR to try and prompt them to fix it quicker. --- 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #12: QPIDJMS-315: Add support for Netty KQueue transport
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/12 Agree with @gemmellr on this, the checks are better provided to Netty for inclusion in the next release so that they can be maintained and matured along with the Epoll and KQueue implementations. In the case where the client emits an unwanted warning for Epoll on 32bit arch we already have a simple workaround which is to provide the URI option to disable the Epoll bits from being 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms pull request #9: QPIDJMS-294: Ensure that SASL mechanism has comple...
Github user tabish121 commented on a diff in the pull request:
https://github.com/apache/qpid-jms/pull/9#discussion_r127257588
--- Diff:
qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/CramMD5MechanismTest.java
---
@@ -85,4 +115,21 @@ public String getName() {
}
}));
}
+
+@Test
+public void testIncompleteExchange() throws Exception {
+Mechanism mechanism = new CramMD5Mechanism();
+
+mechanism.getInitialResponse();
+
+try {
+mechanism.verifyComplete();
+fail("Exception not thrown");
+}
+catch (SaslException e)
--- End diff --
Please fix your code formatting to match the rest of the client code.
---
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.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms pull request #9: QPIDJMS-294: Ensure that SASL mechanism has comple...
Github user tabish121 commented on a diff in the pull request:
https://github.com/apache/qpid-jms/pull/9#discussion_r127257712
--- Diff:
qpid-jms-client/src/main/java/org/apache/qpid/jms/sasl/CramMD5Mechanism.java ---
@@ -86,6 +86,14 @@ public String getName() {
}
@Override
+public void verifyComplete() throws SaslException {
+if (!_sentResponse)
--- End diff --
Please fix your code formatting to match the rest of the client code.
---
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.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms pull request #9: QPIDJMS-294: Ensure that SASL mechanism has comple...
Github user tabish121 commented on a diff in the pull request:
https://github.com/apache/qpid-jms/pull/9#discussion_r127257449
--- Diff:
qpid-jms-client/src/test/java/org/apache/qpid/jms/sasl/AbstractScramSHAMechanismTestBase.java
---
@@ -137,4 +139,25 @@ public void testServerSignatureDiffer() throws
Exception {
// PASS
}
}
+
+@Test
+public void testIncompleteExchange() throws Exception {
+Mechanism mechanism = getConfiguredMechanism();
+
+byte[] clientInitialResponse = mechanism.getInitialResponse();
+assertArrayEquals(expectedClientInitialResponse,
clientInitialResponse);
+
+byte[] clientFinalMessage =
mechanism.getChallengeResponse(serverFirstMessage);
+assertArrayEquals(expectedClientFinalMessage, clientFinalMessage);
+
+try {
+mechanism.verifyComplete();
+fail("Exception not thrown");
+}
+catch (SaslException e)
--- End diff --
Please fix your code formatting to match the rest of the client code.
---
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.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms issue #6: Added two new example classes, Client and Server.
Github user tabish121 commented on the issue: https://github.com/apache/qpid-jms/pull/6 Agree with Robbie on this one, in general if your example requires more than a couple minutes for the reader to digest then you start to lose them. Syncing up with the other examples listed seems like a nice to have goal with interoperability between them being a great final outcome from 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton-j issue #6: PROTON-1410 Moving ThreadLocal logic for Decoders on...
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton-j/pull/6 Personally this doesn't seem a necessary addition to me. I prefer to keep the management of the encoder and decoder instances in the Qpid JMS code and wouldn't use this myself. It adds support code that then has to be maintained as part of the proton-j code that just means we have to manage it later if the Encoders were say made to not require being managed as thread locals for instance, e.g. we made them stateless --- 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton-j issue #5: Small improvements I will need for Artemis
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton-j/pull/5 @clebertsuconic while that may be true, since your initial PR didn't reference either issue, none of that discussion gets logged into the JIRA which is the bit you seem to be missing --- 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton-j issue #5: Small improvements I will need for Artemis
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton-j/pull/5 Please follow the same recommendations that you'd make for someone submitting a PR to Artemis. If you raise the PR with the JIRA key in the title, a bot will raise a note against the JIRA for open/close and also relay future comments to the JIRA. Also, having one PR per issue will allow separate comments on the merits of each with less confusion --- 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton-j issue #5: Small improvements I will need for Artemis
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton-j/pull/5 So you've created two JIRAs but only created one pull request on not reference either JIRA in 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton-j issue #5: Small improvements I will need for Artemis
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton-j/pull/5 There seems to be changes in the MessageImpl that don't have anything to do with the issue at hand and probably would deserve their own 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-proton issue #82: optimizations on Decoder/Encoder
Github user tabish121 commented on the issue: https://github.com/apache/qpid-proton/pull/82 Would suggest removing the author tags from the new files, they are discouraged by the ASF --- 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] qpid-jms pull request: Implements QPIDJMS-164 - Provide an OSGi bu...
Github user tabish121 commented on the pull request: https://github.com/apache/qpid-jms/pull/2#issuecomment-204055703 I did add the BND plugin to proton-j recently in order to generate bundle information in the next release, a review from someone more versed in OSGi would be good. The client would work with any Netty version in the 4.0.x series given our past testing. --- 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. --- - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
