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 2021/06/02 19:54:03 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
clebertsuconic opened a new pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#issuecomment-853342591
@franz1981 ^
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644367543
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
+ if (updateCallback != null) {
+ updateCallback.accept(id, false);
Review comment:
done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644337956
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1129,15 +1140,17 @@ public void appendDeleteRecord(final long id, final boolean sync, final IOComple
checkJournalIsLoaded();
lineUpContext(callback);
- checkKnownRecordID(id, true);
-
- internalAppendDeleteRecord(id, sync, callback);
+ SimpleFuture<Boolean> future = new SimpleFutureImpl<>();
Review comment:
this is not used at all.. it is just for tests.
Besides there's no change here.. the previous version was doing exactly the same.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644325970
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
Review comment:
Save the compactor in a local final variable to save it load it each time
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
+ if (updateCallback != null) {
+ updateCallback.accept(id, false);
+ }
+ logger.debug("Record " + id + " had not been found");
+
Review comment:
Debug if is debug level to save string creation
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
+ if (updateCallback != null) {
+ updateCallback.accept(id, false);
Review comment:
Michael P spent some time to save boxed Long instances to be allocated and I think we should continue here: let's create a specific interface that can use primitive long and boolean
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1129,15 +1140,17 @@ public void appendDeleteRecord(final long id, final boolean sync, final IOComple
checkJournalIsLoaded();
lineUpContext(callback);
- checkKnownRecordID(id, true);
-
- internalAppendDeleteRecord(id, sync, callback);
+ SimpleFuture<Boolean> future = new SimpleFutureImpl<>();
Review comment:
I would create a specific future that can act as a callback too here: it would save the lambda allocation
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
Save compactor in a local variable to save it to be loaded more then one time
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
Yes but it still an unnecessary cost: each time we use it, the jit will load it and null check it implicitly
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644339582
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
Yes but it still an unnecessary cost: each time we use it, the jit will load it and null check it implicitly
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644337348
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
Review comment:
the compactor is only changed on writeLock. it is safe to just use the compactor directly here.
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
compactor is only changed on writeLock(). it is safe to use it here.
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1129,15 +1140,17 @@ public void appendDeleteRecord(final long id, final boolean sync, final IOComple
checkJournalIsLoaded();
lineUpContext(callback);
- checkKnownRecordID(id, true);
-
- internalAppendDeleteRecord(id, sync, callback);
+ SimpleFuture<Boolean> future = new SimpleFutureImpl<>();
Review comment:
this is not used at all.. it is just for tests.
Besides there's no change here.. the previous version was doing exactly the same.
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
+ if (updateCallback != null) {
+ updateCallback.accept(id, false);
Review comment:
done
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
I don't think that's a big deal but I will do it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#issuecomment-853919960
@franz1981 I am merging this.. if you have any further changes.. we can always send new commits...
Tests are solid here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644337631
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
compactor is only changed on writeLock(). it is safe to use it here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644337348
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
Review comment:
the compactor is only changed on writeLock. it is safe to just use the compactor directly here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644325970
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
Review comment:
Save the compactor in a local final variable to save it load it each time
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
+ if (updateCallback != null) {
+ updateCallback.accept(id, false);
+ }
+ logger.debug("Record " + id + " had not been found");
+
Review comment:
Debug if is debug level to save string creation
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1075,14 +1067,27 @@ private void internalAppendUpdateRecord(long id,
Persister persister,
Object record,
boolean sync,
+ BiConsumer<Long, Boolean> updateCallback,
IOCompletion callback) throws InterruptedException, java.util.concurrent.ExecutionException {
- final SimpleFuture<Boolean> result = newSyncAndCallbackResult(sync, callback);
appendExecutor.execute(new Runnable() {
@Override
public void run() {
journalLock.readLock().lock();
try {
JournalRecord jrnRecord = records.get(id);
+ if (jrnRecord == null) {
+ if (compactor == null || (!compactor.containsRecord(id))) {
+ if (updateCallback != null) {
+ updateCallback.accept(id, false);
Review comment:
Michael P spent some time to save boxed Long instances to be allocated and I think we should continue here: let's create a specific interface that can use primitive long and boolean
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1129,15 +1140,17 @@ public void appendDeleteRecord(final long id, final boolean sync, final IOComple
checkJournalIsLoaded();
lineUpContext(callback);
- checkKnownRecordID(id, true);
-
- internalAppendDeleteRecord(id, sync, callback);
+ SimpleFuture<Boolean> future = new SimpleFutureImpl<>();
Review comment:
I would create a specific future that can act as a callback too here: it would save the lambda allocation
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
Save compactor in a local variable to save it to be loaded more then one time
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#discussion_r644833246
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java
##########
@@ -1097,26 +1102,32 @@ public void run() {
// 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());
+ if (compactor != null) {
Review comment:
I don't think that's a big deal but I will do it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3605: ARTEMIS-3327 removing unecessary blocking operations on update and de…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3605:
URL: https://github.com/apache/activemq-artemis/pull/3605#issuecomment-853919960
@franz1981 I am merging this.. if you have any further changes.. we can always send new commits...
Tests are solid here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org