[GitHub] activemq-artemis pull request #1624: ARTEMIS-1489 Adding Timed Buffer into C...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1624: ARTEMIS-1489 Adding Timed Buffer into C...

pgfox
GitHub user clebertsuconic opened a pull request:

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

    ARTEMIS-1489 Adding Timed Buffer into Critical Analyzer

   

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

    $ git pull https://github.com/clebertsuconic/activemq-artemis critical

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

    https://github.com/apache/activemq-artemis/pull/1624.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 #1624
   
----
commit 094560208634560ea240e1f963148466c222ca90
Author: Clebert Suconic <[hidden email]>
Date:   2017-10-30T16:10:59Z

    ARTEMIS-1489 Adding Timed Buffer into Critical Analyzer

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @clebertsuconic You need to be a bit smarter here.  If the TimedBuffer component is locked up, the broker can not sync() to disk.  This will prevent the broker from shutting down gracefully.  The component should shutdown with criticalIOError.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @mtaylor Probably instead of using an instrinsic lock (aka `synchronize`) would be better to use a `ReentrantLock` that has different utilities to check if a lock is acquired and could avoid deadlocks/starvation using [tryLock](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantLock.html#tryLock(long,%20java.util.concurrent.TimeUnit)): if it will wail with a specified timeout than a critical error could be called.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
In reply to this post by pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @franz1981 I'm not sure what you are referring to.  There's no place in this commit that uses a synchronize?  Are you suggesting a generic improvement throughout the code base or something specific?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @mtaylor each TimedBuffer method is marked as synchronized and in particular the flush is being called by a background thread that could race with checkSize if the buffer isn't big enough: any of them could lead to potential deadlocks...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
In reply to this post by pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @franz1981 Those seem like good observations.  Though not really related to this PR.  The idea here is to add TimedBuffer to the critical analyzer, not solve any potential issues with the TimedBuffer itself.  Perhaps you could open a new JIRA with the improvements you've suggested?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
In reply to this post by pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @clebertsuconic After looking into this, it starts getting rather complex, to catch all the edge cases in the various components.  e.g. What happens if the shutdown thread is blocked as a component can't shutdown.  Perhaps just a timeout on shutdown is enough to solve this problem.  I have a test (still failing).  Please take a look.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1624: ARTEMIS-1489 Adding Timed Buffer into Critical...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1624
 
    @mtaylor that's why we use halt by default... you can't shutdown the server if your device can't release a lock.
   
    This task here is pretty simple.. just adding the CriticalAnalyzer to the timed buffer...
   
    We can't go rogue and change this component that much. When you are syncing with AIO it's just for the time where the submit is being made. with NIO we could perhaps open the buffer to be written (it would be a very minor optimization) since the file itself can't be written while another thread is calling sync.
   
    If you could just merge this please.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1624: ARTEMIS-1489 Adding Timed Buffer into C...

pgfox
In reply to this post by pgfox
Github user asfgit closed the pull request at:

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


---