[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
GitHub user CNNJYB opened a pull request:

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

    ARTEMIS-2215 largemessage have been consumed but not deleted

    During the backup and live synchronization, the client consumes the largemessage, then the live crash(the performCachedLargeMessageDeletes method is not executed), after the live startup, the largemessages that have been consumed are not deleted from the disk.

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

    $ git pull https://github.com/CNNJYB/activemq-artemis dev-largemessagenotdelete

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

    https://github.com/apache/activemq-artemis/pull/2483.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 #2483
   
----
commit bf28c751af152956d1a12aa2237502c0a14fc4e8
Author: yb <17061955@...>
Date:   2018-12-29T08:09:48Z

    ARTEMIS-2215 largemessage have been consumed but not deleted from the disk during backup and live sync

----


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244470771
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -461,8 +462,7 @@ void deleteLargeMessageFile(final LargeServerMessage largeServerMessage) throws
              try {
                 if (isReplicated() && replicator.isSynchronizing()) {
                    synchronized (largeMessagesToDelete) {
    --- End diff --
   
    If it's now using CHM there is no need to sync on it


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244470859
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
    @@ -193,7 +193,7 @@ public static JournalContent getType(byte type) {
     
        protected final Map<SimpleString, PersistedAddressSetting> mapPersistedAddressSettings = new ConcurrentHashMap<>();
     
    -   protected final Set<Long> largeMessagesToDelete = new HashSet<>();
    +   protected final Map<Long, LargeServerMessage> largeMessagesToDelete = new ConcurrentHashMap<>();
    --- End diff --
   
    It is possible to use a primitive version of the map ie using primitive longs instead of boxed types


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user CNNJYB commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244657469
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
    @@ -193,7 +193,7 @@ public static JournalContent getType(byte type) {
     
        protected final Map<SimpleString, PersistedAddressSetting> mapPersistedAddressSettings = new ConcurrentHashMap<>();
     
    -   protected final Set<Long> largeMessagesToDelete = new HashSet<>();
    +   protected final Map<Long, LargeServerMessage> largeMessagesToDelete = new ConcurrentHashMap<>();
    --- End diff --
   
    @franz1981 modified, please review, Thanks.


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244685607
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -461,8 +462,7 @@ void deleteLargeMessageFile(final LargeServerMessage largeServerMessage) throws
              try {
                 if (isReplicated() && replicator.isSynchronizing()) {
                    synchronized (largeMessagesToDelete) {
    --- End diff --
   
    @franz1981 looking at the code, looks like its just vanilla HM
   
       protected final Map<Long, LargeServerMessage> largeMessagesToDelete = new HashMap<>();



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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244686021
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java ---
    @@ -193,7 +193,7 @@ public static JournalContent getType(byte type) {
     
        protected final Map<SimpleString, PersistedAddressSetting> mapPersistedAddressSettings = new ConcurrentHashMap<>();
     
    -   protected final Set<Long> largeMessagesToDelete = new HashSet<>();
    +   protected final Map<Long, LargeServerMessage> largeMessagesToDelete = new HashMap<>();
    --- End diff --
   
    @CNNJYB we have our own org.apache.activemq.artemis.utils.collections.LongConcurrentHashMap, this allows it to be concurrent safe, and also means it can be a primitive long.


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244756852
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -309,16 +309,17 @@ public void run() {
         */
        @Override
        protected void performCachedLargeMessageDeletes() {
    -      for (Long largeMsgId : largeMessagesToDelete) {
    -         SequentialFile msg = createFileForLargeMessage(largeMsgId, LargeMessageExtension.DURABLE);
    +      for (LargeServerMessage largeServerMessage : largeMessagesToDelete.values()) {
    --- End diff --
   
    Calling values actually creates a new List, if you're iterating the objects, simply call using forEach method on the collection.


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244757759
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -309,16 +309,17 @@ public void run() {
         */
        @Override
        protected void performCachedLargeMessageDeletes() {
    -      for (Long largeMsgId : largeMessagesToDelete) {
    -         SequentialFile msg = createFileForLargeMessage(largeMsgId, LargeMessageExtension.DURABLE);
    +      for (LargeServerMessage largeServerMessage : largeMessagesToDelete.values()) {
    --- End diff --
   
    If you wish the collection itself to be iterable, then please add this functionality to LongConcurrentHashMap implementation, it shouldn;t be too hard, as already it has a forEach method


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user CNNJYB commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244976446
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -309,16 +309,17 @@ public void run() {
         */
        @Override
        protected void performCachedLargeMessageDeletes() {
    -      for (Long largeMsgId : largeMessagesToDelete) {
    -         SequentialFile msg = createFileForLargeMessage(largeMsgId, LargeMessageExtension.DURABLE);
    +      for (LargeServerMessage largeServerMessage : largeMessagesToDelete.values()) {
    --- End diff --
   
    @michaelandrepearce update, please review, Thanks.


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r244977454
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -727,7 +725,7 @@ private void checkAndCreateDir(final File dir, final boolean create) {
           List<Long> idList = new ArrayList<>();
           for (String filename : filenames) {
              Long id = getLargeMessageIdFromFilename(filename);
    --- End diff --
   
    you can just use a primitive `long` here


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2483#discussion_r245090041
 
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java ---
    @@ -309,16 +309,17 @@ public void run() {
         */
        @Override
        protected void performCachedLargeMessageDeletes() {
    -      for (Long largeMsgId : largeMessagesToDelete) {
    -         SequentialFile msg = createFileForLargeMessage(largeMsgId, LargeMessageExtension.DURABLE);
    +      for (LargeServerMessage largeServerMessage : largeMessagesToDelete.values()) {
    --- End diff --
   
    Usage of LongConcurrentHashMap  looks much better.


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

[GitHub] activemq-artemis issue #2483: ARTEMIS-2215 largemessage have been consumed b...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2483
 
    Im less familar with the journal stuff in general from what i can tell though it looks good, so will let @franz1981 give a thumbs up on the logic change, but from better use of collection libs +1 one me.


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

[GitHub] activemq-artemis pull request #2483: ARTEMIS-2215 largemessage have been con...

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

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


---