Quantcast

[GitHub] activemq-artemis pull request #1268: ARTEMIS-1162: Make new TimedBuffer Conf...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1268: ARTEMIS-1162: Make new TimedBuffer Conf...

gnodet-2
GitHub user michaelandrepearce opened a pull request:

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

    ARTEMIS-1162: Make new TimedBuffer Configurable

    Make new Adapting TimedBuffer and old Fixed TimedBuffer configurable.
   
    Rename new version of TimedBuffer to AdaptingTimedBuffer (and tests)
    Add back old version of TimedBuffer to FixedTimedBuffer (and tests)
    Extract shared public interface
    Add configuration option to toggle between which TimedBuffer to user, in broker.xml "journal-timed-buffer-type=[FIXED,ADAPTING]"

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

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1162

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

    https://github.com/apache/activemq-artemis/pull/1268.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 #1268
   
----
commit cd7ca0a16bba97f87e422869f1818a6e86396189
Author: Michael Andre Pearce <[hidden email]>
Date:   2017-05-13T07:25:39Z

    ARTEMIS-1162: Make new Adapting TimedBuffer and old Fixed TimedBuffer configurable
   
    Rename new version of TimedBuffer to AdaptingTimedBuffer (and tests)
    Add back old version of TimedBuffer to FixedTimedBuffer (and tests)
    Extract shared public interface
    Add configuration option to toggle between which TimedBuffer to user, in broker.xml "journal-timed-buffer-type=[FIXED,ADAPTING]"

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @franz1981 I hope you don't mind me doing this, just as the new timed buffer work you've done, whilst benchmarked we won't get any proper real use feedback till systems hit production, and just want to mitigate risks.
   
    Also it will make less risky other changes, such as the IOLimiter.
   
    Idea would be to add an extra timed buffer type enum, for your IOLimiter version also.



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce Nope, no problem for me...but I'm not 100% sure is the best way to do it: changing the new one to be like the original one is a matter of 1 line change and a boolean flag to be put in the right spot.
    Instead of having:
    ```
    if (checkpoint || currentPendingSyncs >= estimatedOptimalBatch) {
    ```
    Need to be changed into:
    ```
    if (checkpoint || (smartCoalescing && currentPendingSyncs >= estimatedOptimalBatch)) {
    ```
    Finished.
    Now you have one implementation that cover the 2 behaviours.
    wdyt?
   



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce And about loosing some tests, it is not a big deal; the deleted ones has some issues, so it has no practical effect to have them deleted.
    On https://github.com/franz1981/activemq-artemis/tree/timed_buffer_iops_test the [shouldNotFlusgUntilTimeout test](https://github.com/franz1981/activemq-artemis/blob/12c34475f16a73d669c3307999e6fa03601c6cb3/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/journal/impl/TimedBufferTest.java#L137-L137) shows that with the original implementation was possible to go out of IOPS too.
   



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    I actually started with a boolean flag option, but then thought about the other bits being added and before we know it we'd have quite a few flags which equate to lots of config options to be added in broker.xml
   
    Where as having an enum from a user perspective means you simply define which timed buffer type you want, and is a little more plain English with what you're selecting.
   
    Maybe I can see if I can make the enum for config so we don't explode the config paeans but re work the reintroduction of the old behaviour like you propose.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    "paeans" wow my iphone is doing its best today!
   
    I meant parameters


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    For me is ok to have an enum, but I mean that it doesn't make sense to have 2 TimedBuffer implementations.
    You can have one with a flag in the constructor to disable the coalescing: this way the behaviours will be the same as before, but with off heap batch buffer + bulk copy too.
    Then you can you an enum to set that flag or not: it is simpler to maintain IMHO.
    It is different if you want to maintain the older things that wasn't good (heap buffers + no bulk copy)...this is what you're proposing?



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    I agree with you, thanks for the comments :) it's always good to have the second opinions :). Let me re work it and I'll msg once ready for another review :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1268: ARTEMIS-1162: Make new TimedBuffer Conf...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce closed the pull request at:

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


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce I've updated the dev list post with the latest results on the new TimedBuffer vs old and with/without the IO limiter to let other devs to ask/propose/give advice about them if possible.
    As you said, is always good to have more opinions :100:


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce  I removed that update because I've noticed that writing on dev list using Gmail produce better images than using nabble to do the same :(


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @franz1981 I did re work this, but based on the discussion thread i may kill it and the jira as clement is for an all or nothing approach, but with the caveat of having a lot of testing some.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce I don't know honestly how to deal with it: effectively I didn't see any regressions with maxed out writes on disks (only a couple of SSD right now)...
    So maybe makes sense to test it more and fix it (reverting it? is one option, but not the best one IMHO) if it won't work as expected.
    Right now I've only a great improvement in throughput, especially with concurrent writers...


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @franz1981 and here in lies the issue, that the project doesn't currently have a set of agreed performance metrics for different end 2 end scenarios, that are run automatically and reported each release.
   
    I like micro benchmarks as they help isolate a problem, and quickly test if the changes look promising and the best we have today.
   
    As we move to look at other perf improvements without the end 2 end scenarios reporting perf metrics version to version, some changes may start to even step on each other, e.g. one fix may actually regress another, e.g. the classical, batching can improve throughput very easily but trades it for latency.
   
   



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce
   
    > the project doesn't currently have a set of agreed performance metrics for different end 2 end scenarios, that are run automatically and reported each release
    :100:
    > some changes may start to even step on each other, e.g. one fix may actually regress another
    Having a default load generator with different test cases against agreed metrics to make easier identify evident performance changes is a nice idea indeed!
    IMHO the hardest part (for me! I'm giving space to devs more math skilled than me) is to identify the responsibility of a single point of change in a end 2 end result.
   
    I suspect that having proper performance counters on the broker to be collected by user/devs using their own tests (ie with their SLAs in mind) are an additional tool that could help to identify isolated code changes perf changes.
    I smell this could be a good forum discussion to start some thought around this :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce
   
    > batching can improve throughput very easily but trades it for latency.
   
    Agree for my old experiences, but I admit that...depends :+1:
    https://mechanical-sympathy.blogspot.it/2011/10/smart-batching.html
    https://vanilla-java.github.io/2016/07/09/Batching-and-Low-Latency.html



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    I like the first link, much clearer and concisely written to explain the idea.
   
    Just need to prove and get the stats like in the second :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    FYI I've closed the JIRA as won't fix.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    Happy to hear that!I think that there are few people that could explain that concept sound and clear like Martin :)
    What I've implemented on TimedBuffer is something similar but using the queue of the executor service that push the writes against the buffer :P


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1268: ARTEMIS-1162: Make new TimedBuffer Configurabl...

gnodet-2
In reply to this post by gnodet-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1268
 
    @michaelandrepearce
    Re performance metrics:
    it is a work in progress, but I've built something to provide real time instrumentation on the journal performances: https://github.com/franz1981/activemq-artemis/tree/real_time_journal_latency
   
    Right now I need to:
    - define a better lifecycle of the instrumentation -
    - add a console command that collect the samples using the IPC [RCU](https://lwn.net/Articles/262464/) buffer I've built, translating them in a human readable form
   
    The cool thing is that it could be enabled in production system too with no performance impact.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...