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-4206 Dealing with Large Message Issues / duplicate deliveries / messages hanging on folder #4418

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

clebertsuconic
Copy link
Contributor

you add the property -DARTEMIS_DEBUG_REF=true in your java arguments, And you should see Debug output when the messages are not being removed.

@clebertsuconic clebertsuconic marked this pull request as draft March 30, 2023 03:01
@clebertsuconic
Copy link
Contributor Author

I have been debugging with these messages, and I am making a few adjustments (Missed debug statements... etc)... this is being useful in a scenario where the Cpu Is overloaded and everything is failing on the delivering, leaving messages behind.

it is working as I expected to tell me what is happening, but I'm making a few adjustments.

Copy link
Member

@gemmellr gemmellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know its still draft but as its been a couple days and you keep updating it..some WIP comments.

Copy link
Member

@gemmellr gemmellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right, I didnt like that there were two sets of changes in here, intertwined no less :)

I'm not a huge fan of using the log levels to decide to randomly start changing behaviour and stashing stacktraces, even if the intent is sometimes log them...or of using that mechanism throughout the codebase rather than just inside the message classes as it was originally.


DebugState(String description) {
this.description = description;
debugCrumbs.add(new Exception("registered"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the others have timestamps, seems weird this doesnt.

@clebertsuconic
Copy link
Contributor Author

I can squash 2 commits in one for two different and related issues. I thought it was better to keep separated.

Regarding the trace. I think it's better this way. Say an user starts having issues if we just set the logger we would get what we need.

@clebertsuconic
Copy link
Contributor Author

This mechanism is how I found these issues. I would like to keep this door open for future issues.

@clebertsuconic
Copy link
Contributor Author

The idea of this feature is to be a hidden feature...

So, I am going to use the following logger to enable this type of debugging:

private static final Logger refLogger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass().getName() + ".REF_DEBUG");

@gemmellr
Copy link
Member

gemmellr commented Apr 6, 2023

I can squash 2 commits in one for two different and related issues. I thought it was better to keep separated.

No, lets not squash 'different changes' into one commit (yes I saw, its too late), lets keep different changes separate. In that spirit, the original comment was around lets also try not to make different changes entirely reliant on each other anyway by intertwining and overlapping their multiple commits, such that you cant very simply pick or see the effect of one of the different changes in isolation because the commits were mixed together rather than sequential while adding.

Aside, on the squashed commit log message, it starts telling you how to enable the ref counting debug, and then also ends telling you how to enable the ref counting debug but in a less clear way. Seems like one of them could go.

@gemmellr
Copy link
Member

gemmellr commented Apr 6, 2023

The idea of this feature is to be a hidden feature...

So, I am going to use the following logger to enable this type of debugging:

private static final Logger refLogger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass().getName() + ".REF_DEBUG");

It will certainly be hidden, people will enable it without knowing they are since it is a descendent in the regular class name logger hierarchy. Anyone simply enabling debug logging for the class (or more likely, many classes / further up the hierarchy) will get the change in actual system behaviour of the ref counting debug and saving lots of stacktraces. They would have to know about the change in behaviour and reconfigure the specific ref count logger back off in order to restore the prior behaviour and only add debug output for the normal system behaviour.

@michaelandrepearce
Copy link
Contributor

michaelandrepearce commented Apr 6, 2023

The idea of this feature is to be a hidden feature...

Hmmm this worries me, I think any feature should be something we're open about have the capability documented etc. We shouldn't be hiding things so only select few know about it.

This is ethos of open source Apache in my mind.

Fair enough if it's something a general user may not need to know, call it like advanced troubleshooting or something but do not add something hidden

@gemmellr
Copy link
Member

gemmellr commented Apr 6, 2023

The idea of this feature is to be a hidden feature...

Hmmm this worries me, I think any feature should be something we're open about have the capability documented etc. We shouldn't be hiding things so only select few know about it.

This is ethos of open source Apache in my mind.

Fair enough if it's something a general user may not need to know, call it like advanced troubleshooting or something but do not add something hidden

Yeah, its not a 'feature-feature' in any way, and not something I think a user would normally care about so I dont think I'd ever document it personally. Its 'additional broker developer leak debug/trace details via saved stacktraces'.

@clebertsuconic
Copy link
Contributor Author

@gemmellr I will keep the squash.. they are all related anyways.

@clebertsuconic
Copy link
Contributor Author

clebertsuconic commented Apr 6, 2023

@michaelandrepearce I didn't mean as an actual hidden property. I will have a JIRA open for this that will be part of the release notes.

Just that I won't add it to the documentation... as most users won't care about it.. and hopefully won't need to.

Over the years I have been uncommenting code when I needed to debug things... I think it's time to make that thing as part of the codebase now.

- interrupted message breaking reference counting
After the server writing to the client is interrupted in AMQP, the reference counting was broken what would require the server restarted
in order to cleanup the files of any interrupted sends.

- Removed consumer during large message delivery damaging large messages
If the consumer failed to deliver messages for any reason, the message on the queue would be duplicated. what would wipe out the body of the message
and other journal errors would happen because of this.

extra debug capabilities added into RefCountMessage as part of ARTEMIS-4206 in order to identify these issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants