[GitHub] activemq-artemis pull request #1444: ARTEMIS-1326 testTimeOnTimedBuffer inco...

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

[GitHub] activemq-artemis pull request #1444: ARTEMIS-1326 testTimeOnTimedBuffer inco...

franz1981
GitHub user franz1981 opened a pull request:

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

    ARTEMIS-1326 testTimeOnTimedBuffer incorrect timeout count

    The original test wasn't correctly counting the timeout expiration during the burst of writes.
    Now the test is considering the timeout expirations between 2 consecutive bursts of writes in order to verify if the timeout is correctly handled.

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

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

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

    https://github.com/apache/activemq-artemis/pull/1444.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 #1444
   
----
commit 3d29886724d4b506494a392c20973084af84766b
Author: Francesco Nigro <[hidden email]>
Date:   2017-08-06T09:55:41Z

    ARTEMIS-1326 testTimeOnTimedBuffer incorrect timeout count

----


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

franz1981
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1444
 
    @franz1981 thanks for doing 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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    @franz1981 @michaelandrepearce the test was failing for a missing patch, right?
   
    Was this test still failing sporadically?


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    The test was correct before.
   
    This write here would flush the buffer right away and a new write would be issued:
    https://github.com/apache/activemq-artemis/blob/f12116d5a54fbaa2f2507293bb6d524ec3012fa7/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/journal/impl/TimedBufferTest.java#L195-L196
   
   
    The burst cannot complete in less than the timeout.. otherwise the timed buffer is not timing...
   
    I would revert this change... or provide more info on where it failed.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    The only reason it failed before was because of a non applied patch, and this test actually caught that.. you cannot change the test.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer inco...

franz1981
In reply to this post by franz1981
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1444#discussion_r131669696
 
    --- Diff: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/journal/impl/TimedBufferTest.java ---
    @@ -195,24 +194,22 @@ public void onError(int errorCode, String errorMessage) {
              timedBuffer.addBytes(buff, true, callback);
              Assert.assertTrue(latchFlushed.await(5, TimeUnit.SECONDS));
              latchFlushed.setCount(5);
    -
    -
    -         flushes.set(0);
    -
    -         // Sending like crazy... still some wait (1 millisecond) between each send..
    +         //Sending like crazy... still some wait (1 millisecond) between each send..
    +         //The writes will be performed during a timeout interval (it can take < timeout time to do that!)
              long time = System.currentTimeMillis();
              for (int i = 0; i < 5; i++) {
                 timedBuffer.addBytes(buff, true, callback);
                 Thread.sleep(1);
              }
              Assert.assertTrue(latchFlushed.await(5, TimeUnit.SECONDS));
    -
    +         //The TimedBuffer will wait until timeout expiration to perform any additional write
    --- End diff --
   
    this was already done on this test.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    @clebertsuconic
   
    > The burst cannot complete in less than the timeout.. otherwise the timed buffer is not timing...
   
    A burst can complete in less than a timeout (reading the callback calls in the test) but a new one can be accepted only when the timeout will be expired: that's what a sanity test should verify IMHO.
    Logging the different states of the TimedBuffer in the old and the new version help to understand this little difference.
    Reverting it would make it a step back from the correctness point of view IMHO and features equality between NIO/MAPPED/ASYNCIO, because now (after the last change you suggested me) it is really timing with the correct timeout...
    @mtaylor  @michaelandrepearce have worked around these code paths too, 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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    @franz1981 no, a burst cannot complete in less than a timeout.. otherwise.. for the 10th, 11th time.. you would be breaking the TimedBuffer...
   
   
    if Mapped can work without Timing, use a different buffer approach, don't break the TimedBuffer.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    the point of the TimedBuffer is to limit the writes you can send to storage at a limit.... you write once.. flush... next time you write it must be in the configured timing...
   
    the only reason why the flush could be flushed before time is if the buffer is full, and that is currently contemplated... anything else will be sending more writes (and possibly flushes) than what the device is capable of handling....
   
   
    again.. if Mapped works differently, then don't use TimedBuffer.. but you can't change this semantic here.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    >>but a new one can be accepted only when the timeout will be expired: <<
   
    the test already had a previous write that could finish before the timeout... this one is already the new one.
   
    I guess it wasn't clear on my test.. as there was no comment about that. so that was already part of the test.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    @clebertsuconic
    Probably we use the same term for different things: what do you mean by burst and complete?
    For what I mean if you start calling addBytes several times until timeout is not finished yet and you have a flush method (months test observer) that do not sleep at least timeout time, it is normal that you will have your callbacks called in short time.
    What is important is that the buffer can't allow to flush new data until a new timeout will be expired: it is like the notion of credits for the flow control.
    If you finish the credits immediately you can't have new ones until the timeout will finish and it is exactly what is the definition of IOPS for the disks.
    It is not a matter of MAPPED or not...but a matter of a test that is not emulations any correct disk behaviour: asynchronously o not.



---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    Hi @franz1981,  @clebertsuconic has a good point, the issue i had was because i hadn't rebased actually, and since rebasing i hadn't seen the issue again on PR builds.


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    @franz1981 the test was correct before, there's no reason to make this PR


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    @franz1981 can you please close this PR, and leave this test as is?


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer inco...

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

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


---
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 #1444: ARTEMIS-1326 testTimeOnTimedBuffer incorrect t...

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

    https://github.com/apache/activemq-artemis/pull/1444
 
    I'll close this because seems fixed by: https://github.com/apache/activemq-artemis/commit/27d37b735d2248a6ce81f0a6796e26c1a986cb93



---
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...