You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by pgfox <gi...@git.apache.org> on 2017/12/08 07:11:06 UTC
[GitHub] activemq-artemis pull request #1696: NO-JIRA fixed minor regression and brok...
GitHub user pgfox opened a pull request:
https://github.com/apache/activemq-artemis/pull/1696
NO-JIRA fixed minor regression and broken tests in ActiveMQServerControlImpl
fixed minor regression and broken tests due to refactor in ARTEMIS-1364
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pgfox/activemq-artemis fix_regression
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1696.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 #1696
----
commit 7a11a6063429a329c170f125c5da0be250d1ea1e
Author: Pat Fox <pa...@gmail.com>
Date: 2017-12-07T19:49:25Z
NO-JIRA fixed minor regression and broken tests in ActiveMQServerControlImpl
----
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by RaiSaurabh <gi...@git.apache.org>.
Github user RaiSaurabh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
Thanks, @michaelandrepearce @pgfox for the details provided I will keep this in mind for future.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
- added in @michaelandrepearce commit to the PR
- fixed a few minor check style infringements
- added testing to exercise some of the sorting code
I did not squash the commits as I wanted to keep the commit history to show multiple contributors.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox please do, I also didn’t check for checkstyle and all that stuff.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox @michaelandrepearce This looks reasonable to me. Nice work on catching these issues.
---
[GitHub] activemq-artemis pull request #1696: NO-JIRA fixed minor regression and brok...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1696#discussion_r155723971
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java ---
@@ -1737,8 +1737,12 @@ public String listAddresses(String options, int page, int pageSize) throws Excep
try {
final Set<SimpleString> addresses = server.getPostOffice().getAddresses();
List<AddressInfo> addressInfo = new ArrayList<>();
- for (SimpleString address:addresses) {
- addressInfo.add(server.getPostOffice().getAddressInfo(address));
+ for (SimpleString address : addresses) {
+ AddressInfo info = server.getPostOffice().getAddressInfo(address);
+ //ignore if no longer available
+ if (info != null) {
--- End diff --
Nice check.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@michaelandrepearce Thanks Mike, I updated the PR with your commit. I will do a test run tonight on the new PR.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by RaiSaurabh <gi...@git.apache.org>.
Github user RaiSaurabh commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox
For Sorting to work the JSON field name change was necessary as per me. If you look closely you will notice that these field name as actually used to call the individual tab bean getter method (https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/view/ActiveMQAbstractView.java#L135). The column names are used in this method to call the getter methods. Hence to match the getter names we needed to update the field names. If we revert the names the soring will break.
If you need some more details please let me know.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox Good catch, i think this has picked up a regression in the code, the test has done its job in detecting that change, so i don't think we should change the test, but correct the change that was done in the field names.
---
[GitHub] activemq-artemis pull request #1696: NO-JIRA fixed minor regression and brok...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1696
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
- reverted JSON fields to origin names before #1628
- updated *.js to reflect field name reverts.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@mtaylor Ill look to merge this later today, if you wanted to check it before hand?
---
[GitHub] activemq-artemis pull request #1696: NO-JIRA fixed minor regression and brok...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1696#discussion_r155725477
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java ---
@@ -1879,8 +1879,8 @@ public void testListConsumers() throws Exception {
array = (JsonArray) consumersAsJsonObject.get("data");
Assert.assertEquals("number of consumers returned from query", 2, array.size());
- Assert.assertEquals("check consumers address", addressName1.toString(), array.getJsonObject(0).getString("address"));
- Assert.assertEquals("check consumers address", addressName1.toString(), array.getJsonObject(1).getString("address"));
+ Assert.assertEquals("check consumers address", addressName1.toString(), array.getJsonObject(0).getString("queueAddress"));
--- End diff --
Just for my benefit so it links this with the other PR, https://github.com/apache/activemq-artemis/pull/1628
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@michaelandrepearce Thanks Mike,
I agree about the field names in the JSON objects; IMHO they are really part of the external interface so should not be changed unless there is a very good reason. I will try to revert all names changed in #1628 and update this PR.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox looks good thanks, will merge in a day or two if thats ok? just like to leave it a bit of time just for others to get a chance to skim over it before merging ( nudge @mtaylor )
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox all merged now, thanks for this!
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
API names of stuff does not have to match internal names of stuff. Classical internal model mapped to a DTO.
Eg to make the json, internal fields are mapped into json DTO, I would look at reverse mapping this, to avoid this kind of coupling.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
Hi @RaiSaurabh, great work on #1628.
One minor point, the JSON field name changes as part of that PR causes a test to fail. That sparked the conversation above with Mike. It is not a problem, I am currently updating this PR to change back to the original names.
I just wanted to check if there was a "real" requirement for these field name changes, perhaps I am overlooking it.
Thanks in advance.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
@pgfox have PR'd to your branch https://github.com/pgfox/activemq-artemis/pull/1 suggested change, tbh it be even better if we can enum this up somehow, as there some field method access logic duplication in
toJson, getField and apply (of the predicate)
but right now this deal's with the current problem and maybe we can refactor later.
---
[GitHub] activemq-artemis issue #1696: NO-JIRA fixed minor regression and broken test...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1696
Thanks @michaelandrepearce , @mtaylor
---
[GitHub] activemq-artemis pull request #1696: NO-JIRA fixed minor regression and brok...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1696#discussion_r155723803
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java ---
@@ -1879,8 +1879,8 @@ public void testListConsumers() throws Exception {
array = (JsonArray) consumersAsJsonObject.get("data");
Assert.assertEquals("number of consumers returned from query", 2, array.size());
- Assert.assertEquals("check consumers address", addressName1.toString(), array.getJsonObject(0).getString("address"));
- Assert.assertEquals("check consumers address", addressName1.toString(), array.getJsonObject(1).getString("address"));
+ Assert.assertEquals("check consumers address", addressName1.toString(), array.getJsonObject(0).getString("queueAddress"));
--- End diff --
should keep this simply address IMO.
---