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 2020/03/22 05:16:15 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #3041:
Two JIRAs around Paging and AMQP
clebertsuconic opened a new pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041
These two JIRAs are related but I wanted to keep them separate as commits.
Both changes were part of the same testing.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396490919
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/actors/ProcessorBase.java
##########
@@ -99,45 +98,23 @@ public void shutdown(long timeout, TimeUnit unit) {
}
}
- /**
- * It will wait the current execution (if there is one) to finish
- * but will not complete any further executions
- */
+ /** It will shutdown the executor however it will not wait for finishing tasks*/
public int shutdownNow(Consumer<? super T> onPendingItem) {
//alert anyone that has been requested (at least) an immediate shutdown
requestedForcedShutdown = true;
requestedShutdown = true;
- if (inHandler()) {
- stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
- } else {
- //it could take a very long time depending on the current executing task
- do {
- //alert the ExecutorTask (if is running) to just drain the current backlog of tasks
- final int startState = stateUpdater.get(this);
- if (startState == STATE_FORCED_SHUTDOWN) {
- //another thread has completed a forced shutdown: let it to manage the tasks cleanup
- break;
- }
- if (startState == STATE_RUNNING) {
- //wait 100 ms to avoid burning CPU while waiting and
- //give other threads a chance to make progress
- LockSupport.parkNanos(100_000_000L);
- }
- }
- while (!stateUpdater.compareAndSet(this, STATE_NOT_RUNNING, STATE_FORCED_SHUTDOWN));
- //this could happen just one time: the forced shutdown state is the last one and
- //can be set by just one caller.
- //As noted on the execute method there is a small chance that some tasks would be enqueued
+ if (!inHandler()) {
+ flush(1, TimeUnit.SECONDS);
Review comment:
I would use some named const somewhere to help understand why is 1 second or I won't wait at all
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396608747
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/actors/ProcessorBase.java
##########
@@ -99,45 +98,23 @@ public void shutdown(long timeout, TimeUnit unit) {
}
}
- /**
- * It will wait the current execution (if there is one) to finish
- * but will not complete any further executions
- */
+ /** It will shutdown the executor however it will not wait for finishing tasks*/
public int shutdownNow(Consumer<? super T> onPendingItem) {
//alert anyone that has been requested (at least) an immediate shutdown
requestedForcedShutdown = true;
requestedShutdown = true;
- if (inHandler()) {
- stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
- } else {
- //it could take a very long time depending on the current executing task
- do {
- //alert the ExecutorTask (if is running) to just drain the current backlog of tasks
- final int startState = stateUpdater.get(this);
- if (startState == STATE_FORCED_SHUTDOWN) {
- //another thread has completed a forced shutdown: let it to manage the tasks cleanup
- break;
- }
- if (startState == STATE_RUNNING) {
- //wait 100 ms to avoid burning CPU while waiting and
- //give other threads a chance to make progress
- LockSupport.parkNanos(100_000_000L);
- }
- }
- while (!stateUpdater.compareAndSet(this, STATE_NOT_RUNNING, STATE_FORCED_SHUTDOWN));
- //this could happen just one time: the forced shutdown state is the last one and
- //can be set by just one caller.
- //As noted on the execute method there is a small chance that some tasks would be enqueued
+ if (!inHandler()) {
+ flush(1, TimeUnit.SECONDS);
}
+
+ stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
int pendingItems = 0;
- //there is a small chance that execute() could race with this cleanup: the lock allow an all-or-nothing behaviour between them
- synchronized (tasks) {
- T item;
- while ((item = tasks.poll()) != null) {
- onPendingItem.accept(item);
- pendingItems++;
- }
+
+ T item;
+ while ((item = tasks.poll()) != null) {
Review comment:
I am removing the clear on onAddedTaskifNotRunning
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396605633
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/actors/ProcessorBase.java
##########
@@ -99,45 +98,23 @@ public void shutdown(long timeout, TimeUnit unit) {
}
}
- /**
- * It will wait the current execution (if there is one) to finish
- * but will not complete any further executions
- */
+ /** It will shutdown the executor however it will not wait for finishing tasks*/
public int shutdownNow(Consumer<? super T> onPendingItem) {
//alert anyone that has been requested (at least) an immediate shutdown
requestedForcedShutdown = true;
requestedShutdown = true;
- if (inHandler()) {
- stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
- } else {
- //it could take a very long time depending on the current executing task
- do {
- //alert the ExecutorTask (if is running) to just drain the current backlog of tasks
- final int startState = stateUpdater.get(this);
- if (startState == STATE_FORCED_SHUTDOWN) {
- //another thread has completed a forced shutdown: let it to manage the tasks cleanup
- break;
- }
- if (startState == STATE_RUNNING) {
- //wait 100 ms to avoid burning CPU while waiting and
- //give other threads a chance to make progress
- LockSupport.parkNanos(100_000_000L);
- }
- }
- while (!stateUpdater.compareAndSet(this, STATE_NOT_RUNNING, STATE_FORCED_SHUTDOWN));
- //this could happen just one time: the forced shutdown state is the last one and
- //can be set by just one caller.
- //As noted on the execute method there is a small chance that some tasks would be enqueued
+ if (!inHandler()) {
+ flush(1, TimeUnit.SECONDS);
Review comment:
I was afraid of failures on the testsuite. In a production system the component would be going down anyways.
I will try to remove the condition for good.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3041: Two JIRAs
around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#issuecomment-602742511
If the few comments are being addressed (when makes sense) I'm ok with this, can you merge it your self?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396501262
##########
File path: artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContextTest.java
##########
@@ -154,6 +160,8 @@ private void doOnMessageWithDeliveryException(List<Symbol> sourceSymbols,
rc.onMessage(mockDelivery);
+ Thread.sleep(1000);
Review comment:
that's safe in any env?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] brusdev commented on a change in pull request
#3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396512967
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/OperationContextImpl.java
##########
@@ -43,6 +44,8 @@
*/
public class OperationContextImpl implements OperationContext {
+ private static final Logger logger = Logger.getLogger(OperationContextImpl.class);
Review comment:
is this logger used?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396492060
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/actors/ProcessorBase.java
##########
@@ -99,45 +98,23 @@ public void shutdown(long timeout, TimeUnit unit) {
}
}
- /**
- * It will wait the current execution (if there is one) to finish
- * but will not complete any further executions
- */
+ /** It will shutdown the executor however it will not wait for finishing tasks*/
public int shutdownNow(Consumer<? super T> onPendingItem) {
//alert anyone that has been requested (at least) an immediate shutdown
requestedForcedShutdown = true;
requestedShutdown = true;
- if (inHandler()) {
- stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
- } else {
- //it could take a very long time depending on the current executing task
- do {
- //alert the ExecutorTask (if is running) to just drain the current backlog of tasks
- final int startState = stateUpdater.get(this);
- if (startState == STATE_FORCED_SHUTDOWN) {
- //another thread has completed a forced shutdown: let it to manage the tasks cleanup
- break;
- }
- if (startState == STATE_RUNNING) {
- //wait 100 ms to avoid burning CPU while waiting and
- //give other threads a chance to make progress
- LockSupport.parkNanos(100_000_000L);
- }
- }
- while (!stateUpdater.compareAndSet(this, STATE_NOT_RUNNING, STATE_FORCED_SHUTDOWN));
- //this could happen just one time: the forced shutdown state is the last one and
- //can be set by just one caller.
- //As noted on the execute method there is a small chance that some tasks would be enqueued
+ if (!inHandler()) {
+ flush(1, TimeUnit.SECONDS);
}
+
+ stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
int pendingItems = 0;
- //there is a small chance that execute() could race with this cleanup: the lock allow an all-or-nothing behaviour between them
- synchronized (tasks) {
- T item;
- while ((item = tasks.poll()) != null) {
- onPendingItem.accept(item);
- pendingItems++;
- }
+
+ T item;
+ while ((item = tasks.poll()) != null) {
Review comment:
the synchronize was there for 2 reasons:
1. if we would have started using some multi-producer single-consume queue (the ones on JCTools for example...)
2. onAddedTaskIfNotRunning is using a synchronize to allow all-or-nothing behaviour
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396490919
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/actors/ProcessorBase.java
##########
@@ -99,45 +98,23 @@ public void shutdown(long timeout, TimeUnit unit) {
}
}
- /**
- * It will wait the current execution (if there is one) to finish
- * but will not complete any further executions
- */
+ /** It will shutdown the executor however it will not wait for finishing tasks*/
public int shutdownNow(Consumer<? super T> onPendingItem) {
//alert anyone that has been requested (at least) an immediate shutdown
requestedForcedShutdown = true;
requestedShutdown = true;
- if (inHandler()) {
- stateUpdater.set(this, STATE_FORCED_SHUTDOWN);
- } else {
- //it could take a very long time depending on the current executing task
- do {
- //alert the ExecutorTask (if is running) to just drain the current backlog of tasks
- final int startState = stateUpdater.get(this);
- if (startState == STATE_FORCED_SHUTDOWN) {
- //another thread has completed a forced shutdown: let it to manage the tasks cleanup
- break;
- }
- if (startState == STATE_RUNNING) {
- //wait 100 ms to avoid burning CPU while waiting and
- //give other threads a chance to make progress
- LockSupport.parkNanos(100_000_000L);
- }
- }
- while (!stateUpdater.compareAndSet(this, STATE_NOT_RUNNING, STATE_FORCED_SHUTDOWN));
- //this could happen just one time: the forced shutdown state is the last one and
- //can be set by just one caller.
- //As noted on the execute method there is a small chance that some tasks would be enqueued
+ if (!inHandler()) {
+ flush(1, TimeUnit.SECONDS);
Review comment:
I would use some named const somewhere to help understand why is 1 second
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #3041: Two JIRAs
around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3041: Two JIRAs around Paging and AMQP
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3041: Two JIRAs around Paging and AMQP
URL: https://github.com/apache/activemq-artemis/pull/3041#discussion_r396604235
##########
File path: artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContextTest.java
##########
@@ -154,6 +160,8 @@ private void doOnMessageWithDeliveryException(List<Symbol> sourceSymbols,
rc.onMessage(mockDelivery);
+ Thread.sleep(1000);
Review comment:
I forgot to remove this sleep
----------------------------------------------------------------
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
With regards,
Apache Git Services