-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feature/queue browser remove message #47
base: master
Are you sure you want to change the base?
Feature/queue browser remove message #47
Conversation
src/features/QueueBrowser.js
Outdated
browser.log('Received message: ' + message.getGuaranteedMessageId()); | ||
browser.messages[message.getGuaranteedMessageId()] = message; | ||
// Need to explicitly ack otherwise it will not be deleted from the message router | ||
// message.acknowledge(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work? can the browser actually acknowledge and remove a message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot of left over comments and code from the file I originally copied. I actually intended to make this PR to my forked repository, but here we are :). I'll try to clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you made the PR as a contribution to the main repo! I'm reviewing
src/features/QueueBrowser.js
Outdated
browser.messageBrowser.on(solace.QueueBrowserEventName.MESSAGE, function (message) { | ||
browser.log('Received message: ' + message.getGuaranteedMessageId()); | ||
browser.messages[message.getGuaranteedMessageId()] = message; | ||
// Need to explicitly ack otherwise it will not be deleted from the message router |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Need" is the wrong word here. If you're "browsing" messages you usually leave them on the queue for the "real" consumers to process. I think we should provdie a comment with a bit more guidance
src/features/QueueBrowser.js
Outdated
/** | ||
* Solace Systems Node.js API | ||
* Persistence with Queues tutorial - Queue browser | ||
* Demonstrates receiving persistent messages from a queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
improve notes to say we are browsing messages and why you'd want to do that vs. receive them.
Also what are the expectations here? does it browse one message in the queue? or many?
Thank you @spencerhank for contributing this! I made a few comments above. |
src/features/QueueBrowser.js
Outdated
}); | ||
// Define message received event listener | ||
browser.messageBrowser.on(solace.QueueBrowserEventName.MESSAGE, function (message) { | ||
browser.log('Received message: ' + message.getGuaranteedMessageId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually receive message? It's not really receiving a message as much as its browsing the message, the message is not removed from the queue. Also we can add more information on the browsed message apart from just getGuaranteedMessageId()
No description provided.