[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

pgfox
GitHub user franz1981 opened a pull request:

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

    ARTEMIS-1474 TimedBuffer need sleep error detection measured against the expected timeout

    It contains:
     - customization of the timeout error percentage (for development)
     - refactored logic around constant value evaluations
     - reused (but refactored) the old timeout error detection logic while sleeping

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

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

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

    https://github.com/apache/activemq-artemis/pull/1602.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 #1602
   
----
commit 826ff5aa04270d1413551d8aaebadb2f14058ee9
Author: Francesco Nigro <[hidden email]>
Date:   2017-10-20T12:38:39Z

    ARTEMIS-1474 TimedBuffer need sleep error detection measured against the expected timeout
   
    It contains:
     - customization of the timeout error percentage (for development)
     - refactored logic around constant value evaluations
     - reused (but refactored) the old timeout error detection logic while sleeping

----


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

[GitHub] activemq-artemis issue #1602: ARTEMIS-1474 TimedBuffer need sleep error dete...

pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1602
 
    @clebertsuconic And old PR of mine has changed the timeout error detection: this PR is aiming to reuse the previous logic while maintaining the adaptive (discounted) sleep.


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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145958139
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    --- End diff --
   
    Why do we need another System property?


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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145959061
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    --- End diff --
   
    I would remove the System.property here...
    If this doesn't make sense in the configuration.. probably it's something users won't need to configure.


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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145964727
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    --- End diff --
   
    You're right: TBH it was more for development purposes...
    Next week I've planned to do some benchmarks with persistence enabled and I was thinking that would be nice to have it customizable :P
    How it looks the rest of the refactored logic?


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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145971476
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    +   public static final int DEFAULT_TIMEOUT_ERROR_PERCENTAGE = 50;
    +   private static final double MAX_TIMEOUT_ERROR;
    +
    +   static {
    +      final int errorPercentage = Integer.getInteger(JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME, DEFAULT_TIMEOUT_ERROR_PERCENTAGE);
    +      if (errorPercentage < 0) {
    +         throw new RuntimeException("The sleep error percentage must be >= 0");
    +      }
    +      MAX_TIMEOUT_ERROR = 1 + (errorPercentage / 100);
    --- End diff --
   
    What is this timeout error? what magic number is this?
   
    Before the error was a constant of 50% of the requested nanoTime... I don't understand what is this.


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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145972883
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    +   public static final int DEFAULT_TIMEOUT_ERROR_PERCENTAGE = 50;
    +   private static final double MAX_TIMEOUT_ERROR;
    +
    +   static {
    +      final int errorPercentage = Integer.getInteger(JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME, DEFAULT_TIMEOUT_ERROR_PERCENTAGE);
    +      if (errorPercentage < 0) {
    +         throw new RuntimeException("The sleep error percentage must be >= 0");
    +      }
    +      MAX_TIMEOUT_ERROR = 1 + (errorPercentage / 100);
    --- End diff --
   
    ahah is not a magic number: it is the same as before :)
    The old one was `0,5` ( ie `50%`):
    `max timeout error = 1,5 = 1 + 0,5 = 1 + (50/100)`
   



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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145973529
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    +   public static final int DEFAULT_TIMEOUT_ERROR_PERCENTAGE = 50;
    +   private static final double MAX_TIMEOUT_ERROR;
    +
    +   static {
    +      final int errorPercentage = Integer.getInteger(JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME, DEFAULT_TIMEOUT_ERROR_PERCENTAGE);
    +      if (errorPercentage < 0) {
    +         throw new RuntimeException("The sleep error percentage must be >= 0");
    +      }
    +      MAX_TIMEOUT_ERROR = 1 + (errorPercentage / 100);
    --- End diff --
   
    In the old code was indeed:
    ```
                   // I'm letting the real time to be up to 50% than the requested sleep.
                   if (realTimeSleep > timeout * 1.5) {
                      failedChecks++;
                   }
    ```


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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer need sleep err...

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

    https://github.com/apache/activemq-artemis/pull/1602#discussion_r145974574
 
    --- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/buffer/TimedBuffer.java ---
    @@ -34,6 +34,25 @@
     import org.apache.activemq.artemis.journal.ActiveMQJournalLogger;
     
     public final class TimedBuffer {
    +
    +   /**
    +    * Property name to set the percentage of error allowed while expiring the flush {@code timeout} to happen:
    +    * it can assume any positive value from {@code 0} to {@link Integer#MAX_VALUE}.
    +    * <p>
    +    * By default it is {@link #DEFAULT_TIMEOUT_ERROR_PERCENTAGE} more than the configured {@code timeout}.
    +    */
    +   public static final String JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME = "journal.timeout.error";
    +   public static final int DEFAULT_TIMEOUT_ERROR_PERCENTAGE = 50;
    +   private static final double MAX_TIMEOUT_ERROR;
    +
    +   static {
    +      final int errorPercentage = Integer.getInteger(JOURNAL_TIMEOUT_ERROR_PROPERTY_NAME, DEFAULT_TIMEOUT_ERROR_PERCENTAGE);
    +      if (errorPercentage < 0) {
    +         throw new RuntimeException("The sleep error percentage must be >= 0");
    +      }
    +      MAX_TIMEOUT_ERROR = 1 + (errorPercentage / 100);
    --- End diff --
   
    @franz1981 ok, what is the point of this PR? to have a parameterized parameter to tweak about?
   
    I don't want that kind of property open... users can do weird things and then we have to support it.. so.. keep it a constant here.... this is about making sure sleep is working or not.
   
   
    on which case, the semantic will be same as before.. so I'm not understanding what is the point of the change?


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

[GitHub] activemq-artemis issue #1602: ARTEMIS-1474 TimedBuffer need sleep error dete...

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

    https://github.com/apache/activemq-artemis/pull/1602
 
    I still don't understand the point of this PR...
   
    but in case we go ahead, please keep it as:
   
   
    ```java
    public final class TimedBuffer {
   
       private static final double MAX_TIMEOUT_ERROR_FACTOR = 1.5;
    ```
   
    and use that factor instead of those properties. Keep it Simple!


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

[GitHub] activemq-artemis issue #1602: ARTEMIS-1474 TimedBuffer need sleep error dete...

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

    https://github.com/apache/activemq-artemis/pull/1602
 
    @clebertsuconic
    The point of the PR is to restore the original timeout error detection but use the new discounted sleep (it is the description of the PR!): I can remove the property for the reasons you mentioned, while the rest of code refactoring was done to simplify and/or make explicit/document the existing logic.


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

[GitHub] activemq-artemis issue #1602: ARTEMIS-1474 TimedBuffer need sleep error dete...

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

    https://github.com/apache/activemq-artemis/pull/1602
 
    let me do some tests on this.. don't merge it just yet


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

[GitHub] activemq-artemis issue #1602: ARTEMIS-1474 TimedBuffer need sleep error dete...

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

    https://github.com/apache/activemq-artemis/pull/1602
 
    @clebertsuconic I will change the name of the JIRA in something like this:"TimedBuffer refactored and documented sleep logic" maintaining the original logic with few minor changes on the doc and removal of dead code/condition :+1:  Thanks!



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

[GitHub] activemq-artemis pull request #1602: ARTEMIS-1474 TimedBuffer improved doc a...

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

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


---