[GitHub] commons-rdf pull request #49: Cleanup for FindBugs and PMD warnings in -simp...

[ajs6f]

Github user ajs6f commented on a diff in the pull request:

https://github.com/apache/commons-rdf/pull/49#discussion_r168275645
  
--- Diff: 
commons-rdf-simple/src/main/java/org/apache/commons/rdf/simple/experimental/AbstractRDFParser.java
 ---
@@ -58,8 +60,12 @@
  */
 public abstract class AbstractRDFParser> 
implements RDFParser, Cloneable {
 
-public static final ThreadGroup threadGroup = new ThreadGroup("Commons 
RDF parsers");
-private static final ExecutorService threadpool = 
Executors.newCachedThreadPool(r -> new Thread(threadGroup, r));
+public static final AtomicInteger threadCount = new AtomicInteger();
+private static Thread newThread(Runnable r) {
--- End diff --

That's why I put [the name "Commons RDF Parser 
"](https://github.com/apache/commons-rdf/pull/49/files/f1649e034a2623137434fcc2810f8802d3ee9434#diff-68c2acaf43f1ae22da0bdb4eac55a201R65)
 back in. But like I said, I am fine with whatever- I was just tearing through 
PMD warnings at speed after the comment (I think to the board report) about 
there being a lot of them. Shall I take your comment as a direction to remove 
this change? Happy to...


---

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[GitHub] commons-rdf pull request #49: Cleanup for FindBugs and PMD warnings in -simp...

[stain]

Github user stain commented on a diff in the pull request:

https://github.com/apache/commons-rdf/pull/49#discussion_r168274256
  
--- Diff: 
commons-rdf-simple/src/main/java/org/apache/commons/rdf/simple/experimental/AbstractRDFParser.java
 ---
@@ -58,8 +60,12 @@
  */
 public abstract class AbstractRDFParser> 
implements RDFParser, Cloneable {
 
-public static final ThreadGroup threadGroup = new ThreadGroup("Commons 
RDF parsers");
-private static final ExecutorService threadpool = 
Executors.newCachedThreadPool(r -> new Thread(threadGroup, r));
+public static final AtomicInteger threadCount = new AtomicInteger();
+private static Thread newThread(Runnable r) {
--- End diff --

A final ThreadGroup that is not thread safe.. The whole purpose of it is to 
group threads?

It is true that they should not be used for security purpose, but that is 
not why it is here. The ThreadGroup is mainly useful for debugging as people 
might see these "Commons RDF Parser" tree grouped together in the debugger 
(e.g. in Eclipse) rather than the generic no-name you get from the 
ExecutorService.


---

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[GitHub] commons-rdf pull request #49: Cleanup for FindBugs and PMD warnings in -simp...

[ajs6f]

Github user ajs6f commented on a diff in the pull request:

https://github.com/apache/commons-rdf/pull/49#discussion_r168260775
  
--- Diff: 
commons-rdf-simple/src/main/java/org/apache/commons/rdf/simple/experimental/AbstractRDFParser.java
 ---
@@ -58,8 +60,12 @@
  */
 public abstract class AbstractRDFParser> 
implements RDFParser, Cloneable {
 
-public static final ThreadGroup threadGroup = new ThreadGroup("Commons 
RDF parsers");
-private static final ExecutorService threadpool = 
Executors.newCachedThreadPool(r -> new Thread(threadGroup, r));
+public static final AtomicInteger threadCount = new AtomicInteger();
+private static Thread newThread(Runnable r) {
--- End diff --

Because either FindBugs or PMD (can't remember) which called out 
`ThreadGroup` as not threadsafe, which is correct as far as I know. I am in 
_no_ way wedded to this change and if we can guarantee thread-safety from 
outside the class (or simply document it as not threadsafe) I would be happy to 
pull this change out.


---

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[GitHub] commons-rdf pull request #49: Cleanup for FindBugs and PMD warnings in -simp...

[stain]

Github user stain commented on a diff in the pull request:

https://github.com/apache/commons-rdf/pull/49#discussion_r167732171
  
--- Diff: 
commons-rdf-simple/src/main/java/org/apache/commons/rdf/simple/experimental/AbstractRDFParser.java
 ---
@@ -58,8 +60,12 @@
  */
 public abstract class AbstractRDFParser> 
implements RDFParser, Cloneable {
 
-public static final ThreadGroup threadGroup = new ThreadGroup("Commons 
RDF parsers");
-private static final ExecutorService threadpool = 
Executors.newCachedThreadPool(r -> new Thread(threadGroup, r));
+public static final AtomicInteger threadCount = new AtomicInteger();
+private static Thread newThread(Runnable r) {
--- End diff --

I disagree on this style change, the previous code was much cleaner. Why is 
a method-reference on 4 lines that is only used once better than a very small 
lambda?


---

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[GitHub] commons-rdf pull request #49: Cleanup for FindBugs and PMD warnings in -simp...

[ajs6f]

GitHub user ajs6f opened a pull request:

https://github.com/apache/commons-rdf/pull/49

Cleanup for FindBugs and PMD warnings in -simple and -jena



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ajs6f/commons-rdf JenaSimpleCleanup

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/commons-rdf/pull/49.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 #49


commit f1649e034a2623137434fcc2810f8802d3ee9434
Author: ajs6f 
Date:   2017-12-14T17:37:25Z

Cleanup for FindBugs and PMD warnings in commons-rdf-simple and 
commons-rdf-jena




---

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org