[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

jbertram-2
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 <nigro.fra@...>
Date:   2018-11-03T15:37:26Z

    ARTEMIS-1710 Allow management msgs to exceed global-max-size limit

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...

jbertram-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...

jbertram-2
In reply to this post by jbertram-2
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:
    ![image](https://user-images.githubusercontent.com/13125299/47961493-6c89ce00-e00c-11e8-8225-17067e34eb34.png)
    On the left is master, while on the right is this PR branch :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...

jbertram-2
In reply to this post by jbertram-2
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...

jbertram-2
In reply to this post by jbertram-2
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

jbertram-2
In reply to this post by jbertram-2
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?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

jbertram-2
In reply to this post by jbertram-2
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;
          }
    ```


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2414: ARTEMIS-1710 Allow management msgs to exceed g...

jbertram-2
In reply to this post by jbertram-2
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2414
 
    @franz1981 This looks good to me.  Merging.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

jbertram-2
In reply to this post by jbertram-2
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/2414


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

jbertram-2
In reply to this post by jbertram-2
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2414: ARTEMIS-1710 Allow management msgs to e...

jbertram-2
In reply to this post by jbertram-2
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.


---