You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/11/03 17:19:21 UTC
[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2414
ARTEMIS-1710 Allow management msgs to exceed global-max-size limit
This is an alternative version of https://github.com/apache/activemq-artemis/pull/2401 that:
- disable paging on management addresses
- take care to avoid NPE on any uses of a management address `PagingStore`
- management messages are not counted on `PagingManager::getGlobalSize` (given that no `PagingStore` exists for a management address)
- take care to adjust addresses metrics on a management address
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1710_1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2414.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 #2414
----
commit fa715d9c4e49ee66bf367b81b513e21f79c75b93
Author: Francesco Nigro <ni...@...>
Date: 2018-11-03T15:37:26Z
ARTEMIS-1710 Allow management msgs to exceed global-max-size limit
----
---
[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...
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/2414#discussion_r231191592
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
@@ -1244,6 +1244,9 @@ private static PageSubscription locateSubscription(final long queueID,
if (queueInfo != null) {
SimpleString address = queueInfo.getAddress();
PagingStore store = pagingManager.getPageStore(address);
--- End diff --
Might be worth adding some logic to clean this up automatically. Otherwise lots of users probably wont know or might miss it if its a release note.
---
[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2414#discussion_r230684013
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
@@ -1244,6 +1244,9 @@ private static PageSubscription locateSubscription(final long queueID,
if (queueInfo != null) {
SimpleString address = queueInfo.getAddress();
PagingStore store = pagingManager.getPageStore(address);
--- End diff --
@wy96f I'm not following, that method is used purely for the File journal during replication. The null check on paging store is due to, the additional bits in getPageStore(SimpleString storeName).
```java
if (managementAddress != null && storeName.startsWith(managementAddress)) {
return null;
}
```
---
[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2414
---
[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2414
@clebertsuconic @mtaylor @michaelandrepearce
This PR is considering the management addresses as pure broker infrastructure (like any other broker internal mechanics that could allocate heap memory, but is trying to minimize the presence of it.
I'm running a round of the full CI, please take a look of the changes.
I'm providing another version too (probably tomorrow) that is just fixing `PagingStoreImpl`, but TBH this one is a more clean solution, according to the last chats on the original PR.
---
[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2414
I'm checking on the Ci if the failing tests are intermittent ones:
On the left is master, while on the right is this PR branch :+1:
---
[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2414
I have verified that the new test failures are intermittent failures, so this PR seems safe enough although I suppose it need to be reviewed given the amount of changes
---
[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...
Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2414#discussion_r230972615
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
@@ -1244,6 +1244,9 @@ private static PageSubscription locateSubscription(final long queueID,
if (queueInfo != null) {
SimpleString address = queueInfo.getAddress();
PagingStore store = pagingManager.getPageStore(address);
--- End diff --
@mtaylor I'm considering to be compatible with old server. There maybe some old management address paging folders. When we upgrade, getPageStore() may return null. That doesn't matter as we can delete old management folders and restart.
---
[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/2414
Ive reviewed and this looks fine to me. Most of the changes are non intrusive
---
[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/2414
@franz1981 This looks good to me. Merging.
---
[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...
Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2414#discussion_r230676923
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
@@ -1244,6 +1244,9 @@ private static PageSubscription locateSubscription(final long queueID,
if (queueInfo != null) {
SimpleString address = queueInfo.getAddress();
PagingStore store = pagingManager.getPageStore(address);
--- End diff --
private Map<SimpleString, Collection<Integer>> getPageInformationForSync(PagingManager pagingManager) throws Exception {
Map<SimpleString, Collection<Integer>> info = new HashMap<>();
for (SimpleString storeName : pagingManager.getStoreNames()) {
PagingStore store = pagingManager.getPageStore(storeName);
info.put(storeName, store.getCurrentIds());
store.forceAnotherPage();
}
return info;
}
In getPageInformationForSync(), do we need to judge null PagingStore?
---