You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by bennetelli <gi...@git.apache.org> on 2016/08/11 19:54:42 UTC
[GitHub] activemq-artemis pull request #718: just some refactorings
GitHub user bennetelli opened a pull request:
https://github.com/apache/activemq-artemis/pull/718
just some refactorings
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bennetelli/activemq-artemis master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/718.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 #718
----
commit 5e6e0df2d75c5374331adfd448ec316b2343e862
Author: Bennet Schulz <ma...@bennet-schulz.de>
Date: 2016-08-11T18:45:46Z
- reduced visbility of methods and classes
- removed unnesessary unboxing
commit 8768e53cddd2fa929962d2762df961612e05f576
Author: Bennet Schulz <ma...@bennet-schulz.de>
Date: 2016-08-11T19:42:00Z
extracted some methods to get more clear method calls and less duplication
commit e57bd0e16bd6d295663f201daff24d888be6a52c
Author: Bennet Schulz <ma...@bennet-schulz.de>
Date: 2016-08-11T19:42:37Z
removed uncommented lines of 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #718: just some refactorings
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74695394
--- Diff: integration/activemq-vertx-integration/src/main/java/org/apache/activemq/artemis/integration/vertx/ActiveMQVertxLogger.java ---
@@ -38,7 +38,7 @@
* so an INFO message would be 191000 to 191999
*/
@MessageLogger(projectCode = "AMQ")
-public interface ActiveMQVertxLogger extends BasicLogger {
+interface ActiveMQVertxLogger extends BasicLogger {
--- End diff --
If we create separate packages within the module, you won't be able to reuse the package private Logger.
Just because something is not using it now outside of the packet it doesn't mean it eventually couldn't happen IMO
---
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] activemq-artemis pull request #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74622899
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/vertx/ActiveMQVertxUnitTest.java ---
@@ -792,4 +737,36 @@ public void tearDown() throws Exception {
Thread.currentThread().setContextClassLoader(contextClassLoader);
super.tearDown();
}
+
+ private CoreQueueConfiguration createCoreQueueConfiguration(String queueName) {
+ return new CoreQueueConfiguration().setAddress(queueName).setName(queueName);
+ }
+
+ private ConnectorServiceConfiguration createOutgoingConnectorServiceConfiguration(HashMap<String, Object> config,
+ String name) {
+ return new ConnectorServiceConfiguration().setFactoryClassName(VertxOutgoingConnectorServiceFactory.class.getName()).setParams(config).setName(name);
+ }
+
+ private ConnectorServiceConfiguration createIncomingConnectorServiceConfiguration(HashMap<String, Object> config,
+ String name) {
+ return new ConnectorServiceConfiguration().setFactoryClassName(VertxIncomingConnectorServiceFactory.class.getName()).setParams(config).setName(name);
+ }
+
+ private HashMap<String, Object> createIncomingConnectionConfig(String vertxAddress, String incomingQueue) {
+ HashMap<String, Object> config1 = new HashMap<>();
+ config1.put(VertxConstants.HOST, host);
+ config1.put(VertxConstants.PORT, port);
+ config1.put(VertxConstants.VERTX_ADDRESS, vertxAddress);
+ config1.put(VertxConstants.QUEUE_NAME, incomingQueue);
+ return config1;
+ }
+
+ private HashMap<String, Object> createOutgoingConnectionConfig(String queueName, String vertxAddress) {
--- End diff --
Which modules? Do you mean URIs instead of VertxConstants and the config HashMap?
---
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] activemq-artemis pull request #718: just some refactorings
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74492945
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/vertx/ActiveMQVertxUnitTest.java ---
@@ -282,19 +280,7 @@ public void testIncomingEvents() throws Exception {
assertEquals(aShort, recvShort);
//send a JsonObject
- String jsonObjectString = "{\n" +
- "\"Image\": {\n" +
- "\"Width\": 800,\n" +
- "\"Height\": 600,\n" +
- "\"Title\": \"View from 15th Floor\",\n" +
- "\"Thumbnail\": {\n" +
- "\"Url\": \"http://www.example.com/image/481989943\",\n" +
- "\"Height\": 125,\n" +
- "\"Width\": 100\n" +
- "},\n" +
- "\"IDs\": [116, 943, 234, 38793]\n" +
- "}\n" +
- "}";
+ String jsonObjectString = "{\n" + "\"Image\": {\n" + "\"Width\": 800,\n" + "\"Height\": 600,\n" + "\"Title\": \"View from 15th Floor\",\n" + "\"Thumbnail\": {\n" + "\"Url\": \"http://www.example.com/image/481989943\",\n" + "\"Height\": 125,\n" + "\"Width\": 100\n" + "},\n" + "\"IDs\": [116, 943, 234, 38793]\n" + "}\n" + "}";
--- End diff --
In my opinion, unwrapping these lines (and other like this) makes it harder to read/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] activemq-artemis pull request #718: just some refactorings
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74617660
--- Diff: integration/activemq-vertx-integration/src/main/java/org/apache/activemq/artemis/integration/vertx/ActiveMQVertxLogger.java ---
@@ -38,7 +38,7 @@
* so an INFO message would be 191000 to 191999
*/
@MessageLogger(projectCode = "AMQ")
-public interface ActiveMQVertxLogger extends BasicLogger {
+interface ActiveMQVertxLogger extends BasicLogger {
--- End diff --
I don't see a reason to remove the public here, why?
---
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] activemq-artemis pull request #718: just some refactorings
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/718
---
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] activemq-artemis pull request #718: just some refactorings
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74618261
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/vertx/ActiveMQVertxUnitTest.java ---
@@ -792,4 +737,36 @@ public void tearDown() throws Exception {
Thread.currentThread().setContextClassLoader(contextClassLoader);
super.tearDown();
}
+
+ private CoreQueueConfiguration createCoreQueueConfiguration(String queueName) {
+ return new CoreQueueConfiguration().setAddress(queueName).setName(queueName);
+ }
+
+ private ConnectorServiceConfiguration createOutgoingConnectorServiceConfiguration(HashMap<String, Object> config,
+ String name) {
+ return new ConnectorServiceConfiguration().setFactoryClassName(VertxOutgoingConnectorServiceFactory.class.getName()).setParams(config).setName(name);
+ }
+
+ private ConnectorServiceConfiguration createIncomingConnectorServiceConfiguration(HashMap<String, Object> config,
+ String name) {
+ return new ConnectorServiceConfiguration().setFactoryClassName(VertxIncomingConnectorServiceFactory.class.getName()).setParams(config).setName(name);
+ }
+
+ private HashMap<String, Object> createIncomingConnectionConfig(String vertxAddress, String incomingQueue) {
+ HashMap<String, Object> config1 = new HashMap<>();
+ config1.put(VertxConstants.HOST, host);
+ config1.put(VertxConstants.PORT, port);
+ config1.put(VertxConstants.VERTX_ADDRESS, vertxAddress);
+ config1.put(VertxConstants.QUEUE_NAME, incomingQueue);
+ return config1;
+ }
+
+ private HashMap<String, Object> createOutgoingConnectionConfig(String queueName, String vertxAddress) {
--- End diff --
a best refactoring would be to use URIs...
I don't think it's a big deal to make that change here.. I'm mentioning this just in case you start doing a similar thing on other tests.
I have been actually thinking about removing these modules.
---
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] activemq-artemis issue #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on the issue:
https://github.com/apache/activemq-artemis/pull/718
Here the open improvement ticket: https://issues.apache.org/jira/browse/ARTEMIS-680
---
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] activemq-artemis pull request #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74697287
--- Diff: integration/activemq-vertx-integration/src/main/java/org/apache/activemq/artemis/integration/vertx/ActiveMQVertxLogger.java ---
@@ -38,7 +38,7 @@
* so an INFO message would be 191000 to 191999
*/
@MessageLogger(projectCode = "AMQ")
-public interface ActiveMQVertxLogger extends BasicLogger {
+interface ActiveMQVertxLogger extends BasicLogger {
--- End diff --
This would result in making everything public, because anyone perhaps could us it in future.
IMO it should be the other way around. Keep it as private as possible and widen access if needed.
Just say if you want to have it public or not. Will change 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] activemq-artemis pull request #718: just some refactorings
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74695398
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/vertx/ActiveMQVertxUnitTest.java ---
@@ -792,4 +737,36 @@ public void tearDown() throws Exception {
Thread.currentThread().setContextClassLoader(contextClassLoader);
super.tearDown();
}
+
+ private CoreQueueConfiguration createCoreQueueConfiguration(String queueName) {
+ return new CoreQueueConfiguration().setAddress(queueName).setName(queueName);
+ }
+
+ private ConnectorServiceConfiguration createOutgoingConnectorServiceConfiguration(HashMap<String, Object> config,
+ String name) {
+ return new ConnectorServiceConfiguration().setFactoryClassName(VertxOutgoingConnectorServiceFactory.class.getName()).setParams(config).setName(name);
+ }
+
+ private ConnectorServiceConfiguration createIncomingConnectorServiceConfiguration(HashMap<String, Object> config,
+ String name) {
+ return new ConnectorServiceConfiguration().setFactoryClassName(VertxIncomingConnectorServiceFactory.class.getName()).setParams(config).setName(name);
+ }
+
+ private HashMap<String, Object> createIncomingConnectionConfig(String vertxAddress, String incomingQueue) {
+ HashMap<String, Object> config1 = new HashMap<>();
+ config1.put(VertxConstants.HOST, host);
+ config1.put(VertxConstants.PORT, port);
+ config1.put(VertxConstants.VERTX_ADDRESS, vertxAddress);
+ config1.put(VertxConstants.QUEUE_NAME, incomingQueue);
+ return config1;
+ }
+
+ private HashMap<String, Object> createOutgoingConnectionConfig(String queueName, String vertxAddress) {
--- End diff --
There's a way to use URIs to define ActiveMQConnectionFactory 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] activemq-artemis pull request #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74493624
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/vertx/ActiveMQVertxUnitTest.java ---
@@ -282,19 +280,7 @@ public void testIncomingEvents() throws Exception {
assertEquals(aShort, recvShort);
//send a JsonObject
- String jsonObjectString = "{\n" +
- "\"Image\": {\n" +
- "\"Width\": 800,\n" +
- "\"Height\": 600,\n" +
- "\"Title\": \"View from 15th Floor\",\n" +
- "\"Thumbnail\": {\n" +
- "\"Url\": \"http://www.example.com/image/481989943\",\n" +
- "\"Height\": 125,\n" +
- "\"Width\": 100\n" +
- "},\n" +
- "\"IDs\": [116, 943, 234, 38793]\n" +
- "}\n" +
- "}";
+ String jsonObjectString = "{\n" + "\"Image\": {\n" + "\"Width\": 800,\n" + "\"Height\": 600,\n" + "\"Title\": \"View from 15th Floor\",\n" + "\"Thumbnail\": {\n" + "\"Url\": \"http://www.example.com/image/481989943\",\n" + "\"Height\": 125,\n" + "\"Width\": 100\n" + "},\n" + "\"IDs\": [116, 943, 234, 38793]\n" + "}\n" + "}";
--- End diff --
Okay. Will revert it if you want.
---
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] activemq-artemis issue #718: just some refactorings
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/718
Can you squash those commits together so there's not a commit making the change and another commit undoing it? That's a bit messy.
Also, your commit messages should follow the 50/72 format as outlined in the [Hacking Guide](https://github.com/apache/activemq-artemis/blob/master/docs/hacking-guide/en/code.md#typical-development-cycle). It should also reference the 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] activemq-artemis issue #718: just some refactorings
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/718
Also, you have 3 checkstyle violations:
[ERROR] src/main/java/org/apache/activemq/artemis/integration/vertx/OutgoingVertxEventHandler.java:[21,8] (imports) UnusedImports: Unused import - java.util.concurrent.ScheduledExecutorService.
[ERROR] src/main/java/org/apache/activemq/artemis/integration/vertx/OutgoingVertxEventHandler.java:[26,8] (imports) UnusedImports: Unused import - org.apache.activemq.artemis.core.persistence.StorageManager.
[ERROR] src/main/java/org/apache/activemq/artemis/integration/vertx/IncomingVertxEventHandler.java:[20,8] (imports) UnusedImports: Unused import - java.util.concurrent.ScheduledExecutorService.
---
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] activemq-artemis pull request #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74622451
--- Diff: integration/activemq-vertx-integration/src/main/java/org/apache/activemq/artemis/integration/vertx/ActiveMQVertxLogger.java ---
@@ -38,7 +38,7 @@
* so an INFO message would be 191000 to 191999
*/
@MessageLogger(projectCode = "AMQ")
-public interface ActiveMQVertxLogger extends BasicLogger {
+interface ActiveMQVertxLogger extends BasicLogger {
--- End diff --
Because I don't want that anyone else ever is able to use this logger in other packages/tests. Currently it is used within the Vert.x package only and in my opinion it should stay that modular.
---
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] activemq-artemis issue #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on the issue:
https://github.com/apache/activemq-artemis/pull/718
Thanks for the hints @jbertram. Will squash it together and fix the violations as well. Sorry.. didn't know 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] activemq-artemis issue #718: just some refactorings
Posted by johnament <gi...@git.apache.org>.
Github user johnament commented on the issue:
https://github.com/apache/activemq-artemis/pull/718
Could you create a Jira and reference 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] activemq-artemis pull request #718: just some refactorings
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74703757
--- Diff: integration/activemq-vertx-integration/src/main/java/org/apache/activemq/artemis/integration/vertx/ActiveMQVertxLogger.java ---
@@ -38,7 +38,7 @@
* so an INFO message would be 191000 to 191999
*/
@MessageLogger(projectCode = "AMQ")
-public interface ActiveMQVertxLogger extends BasicLogger {
+interface ActiveMQVertxLogger extends BasicLogger {
--- End diff --
I don't really mind on this module.I will merge 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] activemq-artemis pull request #718: just some refactorings
Posted by bennetelli <gi...@git.apache.org>.
Github user bennetelli commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/718#discussion_r74494900
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/vertx/ActiveMQVertxUnitTest.java ---
@@ -282,19 +280,7 @@ public void testIncomingEvents() throws Exception {
assertEquals(aShort, recvShort);
//send a JsonObject
- String jsonObjectString = "{\n" +
- "\"Image\": {\n" +
- "\"Width\": 800,\n" +
- "\"Height\": 600,\n" +
- "\"Title\": \"View from 15th Floor\",\n" +
- "\"Thumbnail\": {\n" +
- "\"Url\": \"http://www.example.com/image/481989943\",\n" +
- "\"Height\": 125,\n" +
- "\"Width\": 100\n" +
- "},\n" +
- "\"IDs\": [116, 943, 234, 38793]\n" +
- "}\n" +
- "}";
+ String jsonObjectString = "{\n" + "\"Image\": {\n" + "\"Width\": 800,\n" + "\"Height\": 600,\n" + "\"Title\": \"View from 15th Floor\",\n" + "\"Thumbnail\": {\n" + "\"Url\": \"http://www.example.com/image/481989943\",\n" + "\"Height\": 125,\n" + "\"Width\": 100\n" + "},\n" + "\"IDs\": [116, 943, 234, 38793]\n" + "}\n" + "}";
--- End diff --
Done.
---
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.
---