You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/12/07 14:38:32 UTC
[GitHub] [activemq-artemis] Yesenkov opened a new pull request, #4308: Update ActiveMQSession.java
Yesenkov opened a new pull request, #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308
When calling Session.recover(), unacknowledged messages must be returned to the queue, that is, ClientSession.rollback (false). If you call ClientSession.rollback (true), the ActiveMQServerMessagePlugin.messageAcknowledged () is called. Which is illogical and misleading. However, this introduces the problem that deliveryCount does not increase on rereads. ut this needs to be fixed elsewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] Yesenkov commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1342597470
> I had to squash all your commits, and all you're doing is to only call true at the rollback(false) in case of individualAck
>
> your concern here is on the call to the plugin itself (based on your texts)?
Well I am writing a test showing an error. If it is not necessary, then it is not necessary, we will not call Session.recover (), we will call Session.getCoreSession().rollback(), which works correctly.
In the case of Session.Recover(), all unacknowledged messages should be returned to the queue, this is not only for the individual acknowledge mode, I just don't care about other modes. That is, in any case, rollback (false) from CoreSession should be called
For unacknowledged messages, ActiveMQServerMessagePlugin.messageAcknowledged (Normal) should not be called, when acknowledged - must - this is important. The same situation in Session.rollback () - ActiveMQServerMessagePlugin.messageAcknowledged (Normal) should not be called.
Current implementation callActiveMQServerMessagePlugin.messageAcknowledged (Normal) on Session.recover () & Session.rollback (). If calling from Session.recover () & Session.rollback () - Session.getCoreSession().rollback(false) - all works corectly, all messages on his right places, and all plugin calls maded correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1342296806
- you need to squash all your commits into a single one, git push -f on your branch
- you need to provide a test
I'm not convinced there's anything broken. you need to provide a test as part of your commit.
or if you are convinced there's an issue, you need to at least provide a way to reproduce and someone would take care of this issue and make a new PR with the test and the proper fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1347465319
>>. Well I am writing a test showing an error.
Where? I did not see your test in any of your commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341281199
Done! I used `andreyyesenkov` for the username as no capital letters and no punctuation is allowed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram closed pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram closed pull request #4308: Update ActiveMQSession.java
URL: https://github.com/apache/activemq-artemis/pull/4308
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] Yesenkov commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341186978
OK!
Please point me where to required test made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341241235
To be clear, this would not qualify as a "NO-JIRA" change. It definitely needs a Jiras as noted previously.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1342300367
I had to squash all your commits, and all you're doing is to only call true at the rollback(false) in case of individualAck
your concern here is on the call to the plugin itself (based on your texts)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341267606
I can create one for you if you provide:
- email address
- preferred username (N.B. hyphens not allowed)
- alternate username (in case the preferred one is already in use)
- display name, if it is different from the username
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1355620984
@Yesenkov, the commit needs to include a test in order to validate the fix is correct and to mitigate against future regressions. Without a test this PR will be closed. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341113749
At the very least this needs both a Jira and an integration test to confirm the fix is valid and mitigate against future regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] Yesenkov commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341242508
Ok!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] Yesenkov commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341211067
Okay, I'll do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341156464
Also, you'll need to squash all your commits and reformat your commit message as detailed in the [hacking guide](https://github.com/apache/activemq-artemis/blob/main/docs/hacking-guide/en/code.md#commitMessageDetails).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341196000
You could probably add your test to `org.apache.activemq.artemis.tests.integration.openwire.amq.JmsTopicRedeliverTest` as it already has a test for `Session.recover()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341213917
You could create a new test called `RecoverTest` in `org.apache.activemq.artemis.tests.integration.jms.client` (i.e. at `tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client`). There are other JMS tests in that package which will give you an idea of how to create your connection, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] Yesenkov commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341253606
But a haven't Jira account, I neet a ticket, or ARTEMIS-XXX identifier
https://infra.apache.org/jira-guidelines.html#who
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] Yesenkov commented on pull request #4308: Update ActiveMQSession.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4308:
URL: https://github.com/apache/activemq-artemis/pull/4308#issuecomment-1341272299
Andrey.Yesenkov@gmail.com
Andrey.Yesenkov@gmail.com - preferred username (N.B. hyphens not allowed)
Andrey.Yesenkov - alternate username (in case the preferred one is already in use)
Andrey Yesenkov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org