You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by jdanekrh <gi...@git.apache.org> on 2018/10/15 09:41:36 UTC
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
GitHub user jdanekrh opened a pull request:
https://github.com/apache/qpid-proton-j/pull/18
NO-JIRA fix use of AtomicInteger and other various small fixes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jdanekrh/qpid-proton-j jd_various_small_fixes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton-j/pull/18.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 #18
----
commit b9c373861159499e999a5d33695cc7f3c1a03ad7
Author: Jiri Danek <jd...@...>
Date: 2018-10-15T09:29:08Z
NO-JIRA fix minor typos (semicolons and javadoc @links)
commit 77d9bc43c3d90e328bcf6baca68e02817fcd8a34
Author: Jiri Danek <jd...@...>
Date: 2018-10-15T09:30:43Z
NO-JIRA fix use of AtomicInteger for class instance IDs
It seems to me the field has to be static, for the code to make sense.
commit 75801e6e003b2db5a97bb07c8da8ddc6f4e32b9b
Author: Jiri Danek <jd...@...>
Date: 2018-10-15T09:34:21Z
NO-JIRA apply IntelliJ suggested fixes where they seem helpful
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA Minor code improvements (e.g. semico...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r231104760
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/message/impl/MessageImpl.java ---
@@ -49,11 +49,7 @@
}
}
- private static final ThreadLocal<EncoderDecoderPair> tlsCodec = new ThreadLocal<EncoderDecoderPair>() {
- @Override protected EncoderDecoderPair initialValue() {
- return new EncoderDecoderPair();
- }
- };
+ private static final ThreadLocal<EncoderDecoderPair> tlsCodec = ThreadLocal.withInitial(EncoderDecoderPair::new);
--- End diff --
I spend about half of yesterday chasing down some sporadic test failures I was seeing in the python test suite, that seemed like it must be from local changes I had in that area. Instead, it eventually turned out to be this change, as I also had this commit in my tree to rebase it. For whatever reason, altering this seems to cause a behaviour shift that the Jython based tests don't like (at least in my env), leading to sporadic failure to run any of the tests. I'll be committing an updated version without this change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225265610
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/TaskImpl.java ---
@@ -29,10 +29,12 @@
import org.apache.qpid.proton.reactor.Task;
public class TaskImpl implements Task, Comparable<TaskImpl> {
+
+ private static final AtomicInteger count = new AtomicInteger();
--- End diff --
Ok, I will create a Jira and send the PR. Will rename to `COUNT`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225101483
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/SslEngineFacadeFactory.java ---
@@ -256,7 +256,7 @@ private static void removeSSLv3Support(final SSLEngine engine)
{
List<String> allowedProtocols = new ArrayList<String>(enabledProtocols);
allowedProtocols.remove(SSLV3_PROTOCOL);
- engine.setEnabledProtocols(allowedProtocols.toArray(new String[allowedProtocols.size()]));
+ engine.setEnabledProtocols(allowedProtocols.toArray(new String[0]));
--- End diff --
ok, this is not really helpful...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #18: NO-JIRA Minor code improvements (e.g. semicolons an...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/18
If the changes aren't related enough to be in the same commit, they probably shouldn't be in the same PR to begin with :)
(My view is that nothing but the most complex layered changes that are fully only understandable step by step should have multiple commits on the same PR.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #18: NO-JIRA fix use of AtomicInteger and other various ...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on the issue:
https://github.com/apache/qpid-proton-j/pull/18
About the granular commits, that is all due to @zkraus ' influence :P I figure squashing is much easier than splitting commits, so it sort of makes sense to start with granular commits...
If I 'd gotten the `AtomicInteger` commit right, I could've directly cherrypicked it into the new PR for the Jira I'll create...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225236948
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/TaskImpl.java ---
@@ -29,10 +29,12 @@
import org.apache.qpid.proton.reactor.Task;
public class TaskImpl implements Task, Comparable<TaskImpl> {
+
+ private static final AtomicInteger count = new AtomicInteger();
--- End diff --
This seems like an actual bug, so it deserves a JIRA. Variable name syntax is now wrong.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225266894
--- Diff: examples/reactor/src/main/java/org/apache/qpid/proton/example/reactor/EchoInputStreamWrapper.java ---
@@ -31,11 +31,12 @@
public class EchoInputStreamWrapper extends Thread {
+ private static final AtomicInteger idCounter = new AtomicInteger();
--- End diff --
Since this is an example, it should show the "best practices". I do not know what the best practices are, though. Inspiring people to use static variables does not to be the right thing. What is there now is not right either...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225224724
--- Diff: examples/reactor/src/main/java/org/apache/qpid/proton/example/reactor/EchoInputStreamWrapper.java ---
@@ -31,11 +31,12 @@
public class EchoInputStreamWrapper extends Thread {
+ private static final AtomicInteger idCounter = new AtomicInteger();
--- End diff --
I don't think this really needed fixed and would have just left it as is. Its only an example, likely to count to 1. Typically having a static shared between reactors would often be incorrect.
If it were to change the variable name would be of the wrong syntax.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA Minor code improvements (e.g. semico...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-proton-j/pull/18
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA Minor code improvements (e.g. semico...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225562577
--- Diff: examples/reactor/src/main/java/org/apache/qpid/proton/example/reactor/EchoInputStreamWrapper.java ---
@@ -31,11 +31,12 @@
public class EchoInputStreamWrapper extends Thread {
+ private static final AtomicInteger idCounter = new AtomicInteger();
--- End diff --
I'd actually be more inclined to delete the example (since its crap) than think about it any more than I have already hehe. Either way, the change should not have been moved to the other PR since it isn't part of that issue.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225263746
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/DefaultSslEngineFacade.java ---
@@ -36,7 +36,7 @@
/**
* Our testing has shown that application buffers need to be a bit larger
* than that provided by {@link SSLSession#getApplicationBufferSize()} otherwise
- * {@link Status#BUFFER_OVERFLOW} will result on {@link SSLEngine#unwrap()}.
+ * {@link Status#BUFFER_OVERFLOW} will result on {@link SSLEngine#unwrap)}.
--- End diff --
typo on my part. I meant to delete both of the parens from the `()`, that was my intention (because there is not a parameterless overload of the function)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA Minor code improvements (e.g. semico...
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r231105833
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/message/impl/MessageImpl.java ---
@@ -49,11 +49,7 @@
}
}
- private static final ThreadLocal<EncoderDecoderPair> tlsCodec = new ThreadLocal<EncoderDecoderPair>() {
- @Override protected EncoderDecoderPair initialValue() {
- return new EncoderDecoderPair();
- }
- };
+ private static final ThreadLocal<EncoderDecoderPair> tlsCodec = ThreadLocal.withInitial(EncoderDecoderPair::new);
--- End diff --
Well, I wouldn't have guessed this is not equivalent change. Another lesson in being conservative and not changing what is known to work without pressing reasons. (Previously I had a bad experience trying to upgrade JUnit version once, with random changes in test results.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #18: NO-JIRA fix use of AtomicInteger and other v...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/18#discussion_r225223601
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/DefaultSslEngineFacade.java ---
@@ -36,7 +36,7 @@
/**
* Our testing has shown that application buffers need to be a bit larger
* than that provided by {@link SSLSession#getApplicationBufferSize()} otherwise
- * {@link Status#BUFFER_OVERFLOW} will result on {@link SSLEngine#unwrap()}.
+ * {@link Status#BUFFER_OVERFLOW} will result on {@link SSLEngine#unwrap)}.
--- End diff --
Seems to me like this change breaks the javadoc rather than fixing anything.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org