[GitHub] activemq-artemis pull request #2458: ARTEMIS-2199 PagingStore leak when dele...

[jbertram]

Github user jbertram commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2458#discussion_r240429222
  
--- Diff: 
artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/impl/JMSServerManagerImpl.java
 ---
@@ -1719,11 +1720,11 @@ public void callback(SimpleString queueName) throws 
Exception {
   @Override
   public void callback(SimpleString address, SimpleString queueName) 
throws Exception {
  Queue queue = server.locateQueue(address);
- Collection bindings = 
server.getPostOffice().getBindingsForAddress(address).getBindings();
--- End diff --

This callback doesn't exist on master so there's no direct counterpart for 
this fix. However, I do plan on reviewing all the usages of 
`getBindingsForAddress` to try to find similar problems.


---

[GitHub] activemq-artemis pull request #2458: ARTEMIS-2199 PagingStore leak when dele...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2458#discussion_r240419559
  
--- Diff: 
artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/impl/JMSServerManagerImpl.java
 ---
@@ -1719,11 +1720,11 @@ public void callback(SimpleString queueName) throws 
Exception {
   @Override
   public void callback(SimpleString address, SimpleString queueName) 
throws Exception {
  Queue queue = server.locateQueue(address);
- Collection bindings = 
server.getPostOffice().getBindingsForAddress(address).getBindings();
--- End diff --

Is there a master counterpart for this fix?


---

[GitHub] activemq-artemis pull request #2458: ARTEMIS-2199 PagingStore leak when dele...

[jbertram]

Github user jbertram closed the pull request at:

https://github.com/apache/activemq-artemis/pull/2458


---

[GitHub] activemq-artemis issue #2458: ARTEMIS-2199 PagingStore leak when deleting qu...

[jbertram]

Github user jbertram commented on the issue:

https://github.com/apache/activemq-artemis/pull/2458
  
I've run the PR build locally and everything worked. I'm pushing this 
directly.


---

Re: [PROPOSAL] Move to gitbox.apache.org

[Daniel Kulp]

> On Dec 10, 2018, at 2:40 PM, Clebert Suconic  
> wrote:
> 
> I'm still trying to understand how it will happen before being able to
> suggest anything.
> 
> In particular what happens with the github mirror?

The only thing that happens is all the GitHub stuff gets enabled.   The merge 
button works.  You can close PR’s, etc….  Other than that, it stays the same. 

Dan


> 
> On Mon, Dec 10, 2018 at 10:31 AM Robbie Gemmell
> mailto:robbie.gemm...@gmail.com>> wrote:
>> 
>> I agree a formal vote isnt needed but a documented consensus on the
>> mailing list is required in order to be included in the initial
>> voluntary moves, which seems to be what Jean-Baptiste is proposing, so
>> it does at minimum need this discussion and probably a direct lazy
>> consensus statement on the matter.
>> 
>> I'd echo your comment about being fine with it any time so long as
>> noone is planning an imminent release.
>> 
>> Robbie
>> 
>> On Fri, 7 Dec 2018 at 17:14, Daniel Kulp > > wrote:
>>> 
>>> 
>>> You don’t need a vote… It’s going to happen one way or another by Feb 7.
>>> 
>>> The only issue is weather to be pro-active and do it now or wait a little 
>>> bit.  If we have releases imminent, I’d say wait till after the 
>>> release.   Otherwise, I’m OK at any time.
>>> 
>>> Dan
>>> 
>>> 
>>> 
 On Dec 7, 2018, at 12:03 PM, Jean-Baptiste Onofré  
 wrote:
 
 Hi all,
 
 Our repositories are currently located on git-wip-us.apache.org.
 
 This service will be decommissioned in the coming month.
 
 I'm proposing to move our repositories to gitbox.apache.org.
 
 I'm volunteer to start a vote, and if OK, I will deal with the infra.
 
 Thoughts ?
 
 Regards
 JB
 --
 Jean-Baptiste Onofré
 jbono...@apache.org
 http://blog.nanthrax.net
 Talend - http://www.talend.com
>>> 
>>> --
>>> Daniel Kulp
>>> dk...@apache.org  >> > - http://dankulp.com/blog 
>>>  >> >
>>> Talend Community Coder - http://talend.com  
>>> >
> 
> 
> 
> -- 
> Clebert Suconic

-- 
Daniel Kulp
dk...@apache.org  - http://dankulp.com/blog 

Talend Community Coder - http://talend.com 

Re: [PROPOSAL] Move to gitbox.apache.org

[Clebert Suconic]

I'm still trying to understand how it will happen before being able to
suggest anything.

In particular what happens with the github mirror?

On Mon, Dec 10, 2018 at 10:31 AM Robbie Gemmell
 wrote:
>
> I agree a formal vote isnt needed but a documented consensus on the
> mailing list is required in order to be included in the initial
> voluntary moves, which seems to be what Jean-Baptiste is proposing, so
> it does at minimum need this discussion and probably a direct lazy
> consensus statement on the matter.
>
> I'd echo your comment about being fine with it any time so long as
> noone is planning an imminent release.
>
> Robbie
>
> On Fri, 7 Dec 2018 at 17:14, Daniel Kulp  wrote:
> >
> >
> > You don’t need a vote… It’s going to happen one way or another by Feb 7.
> >
> > The only issue is weather to be pro-active and do it now or wait a little 
> > bit.  If we have releases imminent, I’d say wait till after the 
> > release.   Otherwise, I’m OK at any time.
> >
> > Dan
> >
> >
> >
> > > On Dec 7, 2018, at 12:03 PM, Jean-Baptiste Onofré  
> > > wrote:
> > >
> > > Hi all,
> > >
> > > Our repositories are currently located on git-wip-us.apache.org.
> > >
> > > This service will be decommissioned in the coming month.
> > >
> > > I'm proposing to move our repositories to gitbox.apache.org.
> > >
> > > I'm volunteer to start a vote, and if OK, I will deal with the infra.
> > >
> > > Thoughts ?
> > >
> > > Regards
> > > JB
> > > --
> > > Jean-Baptiste Onofré
> > > jbono...@apache.org
> > > http://blog.nanthrax.net
> > > Talend - http://www.talend.com
> >
> > --
> > Daniel Kulp
> > dk...@apache.org  - http://dankulp.com/blog 
> > 
> > Talend Community Coder - http://talend.com 



-- 
Clebert Suconic

[GitHub] activemq-artemis pull request #2458: ARTEMIS-2199 PagingStore leak when dele...

[jbertram]

GitHub user jbertram opened a pull request:

https://github.com/apache/activemq-artemis/pull/2458

ARTEMIS-2199 PagingStore leak when deleting queue

When deleting a queue the JMSPostQueueDeletionCallback#callback will
invoke PostOfficeImpl#getBindingsForAddress which will *create* a
Bindings instance if one doesn't exist. This will trigger the creation
of a PagingStore instance which will be stored by the PagingManager
and may never be deleted. This is particularly problematic for use-cases
involving temporary JMS queues.

This change uses the lookupBindingsForAddress instead of
getBindingsForAddress which doesn't implicitly create a Bindings
instance.

This problem doesn't exist on the master branch as the
JMSPostQueueDeletionCallback no longer exists there.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-2199

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2458.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 #2458


commit 6032fce45703d57be7e0a2542b443508674c406a
Author: Justin Bertram 
Date:   2018-12-10T18:13:37Z

ARTEMIS-2199 PagingStore leak when deleting queue

When deleting a queue the JMSPostQueueDeletionCallback#callback will
invoke PostOfficeImpl#getBindingsForAddress which will *create* a
Bindings instance if one doesn't exist. This will trigger the creation
of a PagingStore instance which will be stored by the PagingManager
and may never be deleted. This is particularly problematic for use-cases
involving temporary JMS queues.

This change uses the lookupBindingsForAddress instead of
getBindingsForAddress which doesn't implicitly create a Bindings
instance.

This problem doesn't exist on the master branch as the
JMSPostQueueDeletionCallback no longer exists there.




---

[GitHub] activemq-artemis issue #2287: ARTEMIS-2069 Backup doesn't activate after sha...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2287
  
My point is.  -1 is forever.  If you set it forever it will be failing at 2 
seconds.  


What happens if you just configure the system at 2 seconds instead of -1?


---

[GitHub] activemq-artemis pull request #2287: ARTEMIS-2069 Backup doesn't activate af...

[TomasHofman]

Github user TomasHofman commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2287#discussion_r240294762
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java
 ---
@@ -49,6 +49,8 @@
 
private static final byte NOT_STARTED = 'N';
 
+   private static final long LOCK_ACCESS_FAILURE_WAIT_TIME = 2000;
--- End diff --

I added another test to verify that `lockAcquisitionTimeout` is honored.

The only hard waiting time is 500ms on line 313, but that's the original 
behavior. I can make that honor `lockAcquisitionTimeout` as well if desired.


---

[GitHub] activemq-artemis pull request #2287: ARTEMIS-2069 Backup doesn't activate af...

[TomasHofman]

Github user TomasHofman commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2287#discussion_r240293147
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java
 ---
@@ -49,6 +49,8 @@
 
private static final byte NOT_STARTED = 'N';
 
+   private static final long LOCK_ACCESS_FAILURE_WAIT_TIME = 2000;
--- End diff --

@clebertsuconic no, the `lockAcquisitionTimeout` is not ignored.

If you check the line 337, actual waiting time is either 
`LOCK_ACCESS_FAILURE_WAIT_TIME` or time remaining until 
`lockAcquisitionTimeout` runs out, whichever is lower, so acquisition timeout 
set by user is honored.


---

[GitHub] activemq-artemis pull request #2454: ARTEMIS-2196 Avoid creating RandomAcces...

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2454#discussion_r240290721
  
--- Diff: 
artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
 ---
@@ -91,7 +92,10 @@ public final void delete() throws IOException, 
InterruptedException, ActiveMQExc
  close();
   }
 
-  if (file.exists() && !file.delete()) {
+  try {
+ Files.deleteIfExists(file.toPath());
--- End diff --

It is helpful in difference cases: 

- NFS/GlusterFS volumes because it avoids 2 rount-trip across network
- local storage: is atomic with other file system operations and much 
cheaper re native (eg malloc) allocations on JNI side


---

Re: [PROPOSAL] Move to gitbox.apache.org

[Robbie Gemmell]

I agree a formal vote isnt needed but a documented consensus on the
mailing list is required in order to be included in the initial
voluntary moves, which seems to be what Jean-Baptiste is proposing, so
it does at minimum need this discussion and probably a direct lazy
consensus statement on the matter.

I'd echo your comment about being fine with it any time so long as
noone is planning an imminent release.

Robbie

On Fri, 7 Dec 2018 at 17:14, Daniel Kulp  wrote:
>
>
> You don’t need a vote… It’s going to happen one way or another by Feb 7.
>
> The only issue is weather to be pro-active and do it now or wait a little 
> bit.  If we have releases imminent, I’d say wait till after the release.  
>  Otherwise, I’m OK at any time.
>
> Dan
>
>
>
> > On Dec 7, 2018, at 12:03 PM, Jean-Baptiste Onofré  wrote:
> >
> > Hi all,
> >
> > Our repositories are currently located on git-wip-us.apache.org.
> >
> > This service will be decommissioned in the coming month.
> >
> > I'm proposing to move our repositories to gitbox.apache.org.
> >
> > I'm volunteer to start a vote, and if OK, I will deal with the infra.
> >
> > Thoughts ?
> >
> > Regards
> > JB
> > --
> > Jean-Baptiste Onofré
> > jbono...@apache.org
> > http://blog.nanthrax.net
> > Talend - http://www.talend.com
>
> --
> Daniel Kulp
> dk...@apache.org  - http://dankulp.com/blog 
> 
> Talend Community Coder - http://talend.com 

[GitHub] activemq-artemis pull request #2287: ARTEMIS-2069 Backup doesn't activate af...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2287#discussion_r240232734
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java
 ---
@@ -49,6 +49,8 @@
 
private static final byte NOT_STARTED = 'N';
 
+   private static final long LOCK_ACCESS_FAILURE_WAIT_TIME = 2000;
--- End diff --

You are here effectively ignoring the acquire timeout where the user can 
configure it. I'm not sure this is correct.

your test should play with a configured timeout and see if you get the 
expected behaviour. Adding this you are forcing your own timeout bypassing the 
configured one.


---

[GitHub] activemq-artemis issue #2457: ARTEMIS-2193 Artemis fails on OutOfMemoryError...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2457
  
@clebertsuconic Seems that both drop/fail policies are racing on the large 
message while deleting the record from the journal


---

[GitHub] activemq-artemis issue #2457: ARTEMIS-2193 Artemis fails on OutOfMemoryError...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2457
  
Is this specific to drop ?


---

[GitHub] activemq-artemis issue #2457: ARTEMIS-2193 Artemis fails on OutOfMemoryError...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2457
  
@wy96f @clebertsuconic Guys, you're very deep inside the large messages 
mechanics so I bring this one to your attention, if you can take a look :+1: 


---

[GitHub] activemq-artemis pull request #2457: ARTEMIS-2193 Artemis fails on OutOfMemo...

[franz1981]

GitHub user franz1981 opened a pull request:

https://github.com/apache/activemq-artemis/pull/2457

ARTEMIS-2193 Artemis fails on OutOfMemoryError with fast producers

Large messages pendingRecordID is not accessed atomically, leading
to races that would lead to records that cannot been found on the
journal for deletion: it would lead to cause NPE that won't clean
the pending tasks on the current OperationContextImpl.
Adding a cleanup on error of those tasks and avoiding the race
to happen by adding proper synchronization will both enforce
correct clean up when something bad happen and avoid NPE.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-2193

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2457.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 #2457


commit e35288d0d984ed03b0995c4f6432825acc698b74
Author: Francesco Nigro 
Date:   2018-12-07T08:35:08Z

ARTEMIS-2193 Artemis fails on OutOfMemoryError with fast producers

Large messages pendingRecordID is not accessed atomically, leading
to races that would lead to records that cannot been found on the
journal for deletion: it would lead to cause NPE that won't clean
the pending tasks on the current OperationContextImpl.
Adding a cleanup on error of those tasks and avoiding the race
to happen by adding proper synchronization will both enforce
correct clean up when something bad happen and avoid NPE.




---

[GitHub] activemq-artemis pull request #2456: ARTEMIS-2198 Reduce GC pressure on Tran...

[franz1981]

GitHub user franz1981 opened a pull request:

https://github.com/apache/activemq-artemis/pull/2456

ARTEMIS-2198 Reduce GC pressure on TransactionImpl and 
OperationalContextImpl

TransactionImpl::properties are often not used and could be
avoided to be allocated.
OperationalContextImpl.TaskHolders instances are turned into static
classes to avoid refecencing back the context, making the life
easier for the GC.
OperationalContexImpl volatile loads can be reduced to make the
code faster on the hot path.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-2198

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2456.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 #2456


commit cc68161334f7fb694554b64a4ec621a6423920a5
Author: Francesco Nigro 
Date:   2018-12-07T09:36:29Z

ARTEMIS-2198 Reduce GC pressure on TransactionImpl and 
OperationalContextImpl

TransactionImpl::properties are often not used and could be
avoided to be allocated.
OperationalContextImpl.TaskHolders instances are turned into static
classes to avoid refecencing back the context, making the life
easier for the GC.
OperationalContexImpl volatile loads can be reduced to make the
code faster on the hot path.




---