You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by clebertsuconic <gi...@git.apache.org> on 2016/10/28 04:46:28 UTC
[GitHub] activemq-artemis pull request #872: A few fixes before the release around jo...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/activemq-artemis/pull/872
A few fixes before the release around journal / threading...
This is not ready to be merged yet.. but it's good for reviews already.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/activemq-artemis journal
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/872.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 #872
----
commit 9c7a18e2ee2b5c1481ef871a721344b3b863e379
Author: Clebert Suconic <cl...@apache.org>
Date: 2016-10-24T22:20:20Z
ARTEMIS-829 Removing messages re-encoding
https://issues.apache.org/jira/browse/ARTEMIS-829
commit 7d0e29db5bd9e0dd446b651b22f84ee7fc4d9cb5
Author: Clebert Suconic <cl...@apache.org>
Date: 2016-10-27T21:30:34Z
ARTEMIS-828 Queue browsing can be out of sync while paging
https://issues.apache.org/jira/browse/ARTEMIS-828
commit b3f13ebe3426a44a1f8be6877c797ea9bdd4fae4
Author: barreiro <lb...@gmail.com>
Date: 2016-01-22T03:23:26Z
ARTEMIS-822 Add executor service to JournalImpl for append operations and remove synchronization
https://issues.apache.org/jira/browse/ARTEMIS-822
commit c94d1d8a2c647c290d793f97e338f6f8e0007089
Author: Clebert Suconic <cl...@apache.org>
Date: 2016-10-27T16:32:04Z
ARTEMIS-822 Review journal threading model
https://issues.apache.org/jira/browse/ARTEMIS-822
commit 1b2efb15ff527c67ce30dae8a351ffb2e3d39df9
Author: Clebert Suconic <cl...@apache.org>
Date: 2016-10-28T04:31:42Z
NO-JIRA fixing byteman (extra tests)
Two issues encountered here:
i - ClosingConnectionTest was intermittently breaking other tests, in particular it was breaking PagingLeakTest for no apparent reason
. apparently it was a dead lock from removeAddress on the ServerController
ii - it still showing issues after removing the not needed synchronziation. Since the test is not really needed I am just removing the offending test.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
RE: [GitHub] activemq-artemis issue #872: A few fixes before the
release around journal /...
Posted by "Renumathy B (NCS)" <re...@ncs.com.sg>.
Please , I donot want to receive any emails from now from you.
Can you remove my name from this list.
Thanks and regards
-----Original Message-----
From: clebertsuconic [mailto:git@git.apache.org]
Sent: Friday, 28 October, 2016 12:48 PM
To: dev@activemq.apache.org
Subject: [GitHub] activemq-artemis issue #872: A few fixes before the release around journal /...
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/872
@barreiro I would appreciate your review since you did one of the commits here.
---
If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastructure@apache.org or file a JIRA ticket with INFRA.
---
[GitHub] activemq-artemis issue #872: A few fixes before the release around journal /...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/872
@barreiro I would appreciate your review since you did one of the commits here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #872: A few fixes before the release around jo...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/872#discussion_r85810848
--- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java ---
@@ -688,32 +690,37 @@ public void appendAddRecord(final long id,
final boolean sync,
final IOCompletion callback) throws Exception {
checkJournalIsLoaded();
+ lineUpContext(callback);
+ pendingRecords.add(id);
- journalLock.readLock().lock();
-
- try {
- JournalInternalRecord addRecord = new JournalAddRecord(true, id, recordType, record);
-
- if (callback != null) {
- callback.storeLineUp();
- }
-
- synchronized (lockAppend) {
- JournalFile usedFile = appendRecord(addRecord, false, sync, null, callback);
+ Future<?> result = appendExecutor.submit(new Runnable() {
+ @Override
+ public void run() {
+ journalLock.readLock().lock();
+ try {
+ JournalInternalRecord addRecord = new JournalAddRecord(true, id, recordType, record);
+ JournalFile usedFile = appendRecord(addRecord, false, sync, null, callback);
+ records.put(id, new JournalRecord(usedFile, addRecord.getEncodeSize()));
- if (logger.isTraceEnabled()) {
- logger.trace("appendAddRecord::id=" + id +
- ", userRecordType=" +
- recordType +
- ", record = " + record +
- ", usedFile = " +
- usedFile);
+ if (logger.isTraceEnabled()) {
+ logger.trace("appendAddRecord::id=" + id +
+ ", userRecordType=" +
+ recordType +
+ ", record = " + record +
+ ", usedFile = " +
+ usedFile);
+ }
+ } catch (Exception e) {
+ ActiveMQJournalLogger.LOGGER.error(e.getMessage(), e);
--- End diff --
Shouldn't this have a concrete method rather than just using error()?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #872: A few fixes before the release around journal /...
Posted by barreiro <gi...@git.apache.org>.
Github user barreiro commented on the issue:
https://github.com/apache/activemq-artemis/pull/872
It's good. I also support the removal of the defaultAddress 'optimization'.
@clebertsuconic Thanks for taking care of the loose ends!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #872: A few fixes before the release around journal /...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/872
This is ready to be merged now...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #872: A few fixes before the release around jo...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/872#discussion_r85811440
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/OrderedExecutorFactory.java ---
@@ -104,7 +103,7 @@ public void run() {
// This could happen during shutdowns. Nothing to be concerned about here
logger.debug("Interrupted Thread", e);
} catch (Throwable t) {
- ActiveMQClientLogger.LOGGER.caughtunexpectedThrowable(t);
+ logger.warn(t.getMessage(), t);
--- End diff --
nope... it's just a generic exception, it can just call logger.warn directly
if were doing anything with it, such as adding our messages.. then it would be through the internationalized Logger.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #872: A few fixes before the release around jo...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/872#discussion_r85810957
--- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java ---
@@ -724,94 +731,86 @@ public void appendUpdateRecord(final long id,
final boolean sync,
final IOCompletion callback) throws Exception {
checkJournalIsLoaded();
+ lineUpContext(callback);
+ checkKnownRecordID(id);
- journalLock.readLock().lock();
+ Future<?> result = appendExecutor.submit(new Runnable() {
+ @Override
+ public void run() {
+ journalLock.readLock().lock();
+ try {
+ JournalRecord jrnRecord = records.get(id);
+ JournalInternalRecord updateRecord = new JournalAddRecord(false, id, recordType, record);
+ JournalFile usedFile = appendRecord(updateRecord, false, sync, null, callback);
- try {
- JournalRecord jrnRecord = records.get(id);
+ if (logger.isTraceEnabled()) {
+ logger.trace("appendUpdateRecord::id=" + id +
+ ", userRecordType=" +
+ recordType +
+ ", usedFile = " +
+ usedFile);
+ }
- if (jrnRecord == null) {
- if (!(compactor != null && compactor.lookupRecord(id))) {
- throw new IllegalStateException("Cannot find add info " + id);
+ // record==null here could only mean there is a compactor
+ // computing the delete should be done after compacting is done
+ if (jrnRecord == null) {
+ compactor.addCommandUpdate(id, usedFile, updateRecord.getEncodeSize());
+ } else {
+ jrnRecord.addUpdateFile(usedFile, updateRecord.getEncodeSize());
+ }
+ } catch (Exception e) {
+ ActiveMQJournalLogger.LOGGER.error(e.getMessage(), e);
--- End diff --
Same here as above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #872: A few fixes before the release around jo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/872
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---