Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-2215 largemessage have been consumed but not deleted #2483

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@CNNJYB
Copy link

CNNJYB commented Dec 29, 2018

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.

@CNNJYB CNNJYB force-pushed the CNNJYB:dev-largemessagenotdelete branch 2 times, most recently from 128dcb3 to 9a9b1a1 Dec 29, 2018

@@ -461,8 +462,7 @@ void deleteLargeMessageFile(final LargeServerMessage largeServerMessage) throws
try {
if (isReplicated() && replicator.isSynchronizing()) {
synchronized (largeMessagesToDelete) {

This comment has been minimized.

Copy link
@franz1981

franz1981 Dec 29, 2018

Contributor

If it's now using CHM there is no need to sync on it

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Jan 2, 2019

Contributor

@franz1981 looking at the code, looks like its just vanilla HM

protected final Map<Long, LargeServerMessage> largeMessagesToDelete = new HashMap<>();

to that note probably should be using our own org.apache.activemq.artemis.utils.collections.LongConcurrentHashMap

@@ -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<>();

This comment has been minimized.

Copy link
@franz1981

franz1981 Dec 29, 2018

Contributor

It is possible to use a primitive version of the map ie using primitive longs instead of boxed types

This comment has been minimized.

Copy link
@CNNJYB

CNNJYB Jan 2, 2019

Author

@franz1981 modified, please review, Thanks.

@CNNJYB CNNJYB force-pushed the CNNJYB:dev-largemessagenotdelete branch 2 times, most recently from 9d51281 to c21e08a Dec 29, 2018

@@ -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<>();

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Jan 2, 2019

Contributor

@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.

@CNNJYB CNNJYB force-pushed the CNNJYB:dev-largemessagenotdelete branch from c21e08a to ed54c46 Jan 2, 2019

@@ -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()) {

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Jan 2, 2019

Contributor

Calling values() creates a new List, if you're iterating the objects, simply call using forEach method on the collection.

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Jan 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@CNNJYB

CNNJYB Jan 3, 2019

Author

@michaelandrepearce update, please review, Thanks.

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Jan 3, 2019

Contributor

Usage of LongConcurrentHashMap looks much better.

@CNNJYB CNNJYB force-pushed the CNNJYB:dev-largemessagenotdelete branch from ed54c46 to 0cf0787 Jan 3, 2019

@@ -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);

This comment has been minimized.

Copy link
@franz1981

franz1981 Jan 3, 2019

Contributor

you can just use a primitive long here

@CNNJYB CNNJYB force-pushed the CNNJYB:dev-largemessagenotdelete branch from 0cf0787 to 240cbd9 Jan 3, 2019

yb

@CNNJYB CNNJYB force-pushed the CNNJYB:dev-largemessagenotdelete branch from 240cbd9 to bf057ba Jan 3, 2019

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

michaelandrepearce commented Jan 3, 2019

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.

@asfgit asfgit closed this in a34dc64 Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.