Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use atomic timestamp for sensitive tests. #147

Merged
merged 1 commit into from
May 18, 2020

Conversation

enozcan
Copy link
Contributor

@enozcan enozcan commented May 8, 2020

As discussed in #126, millisecond resolution may fall short when updates are frequent. This behavior can be tolerable in deployments since the cache will restore itself after a cache miss. However in test environment this makes some tests inconsistent. This will not be the case anymore when the new factory is used.

  • Signed Contributor Agreement

@enozcan enozcan requested review from pivovarit and a team as code owners May 8, 2020 23:04
@enozcan enozcan requested review from alparslanavci and removed request for a team and alparslanavci May 8, 2020 23:04
@pivovarit
Copy link
Contributor

pivovarit commented May 9, 2020

Looks good. However, we should revisit the idea of using SimpleTimestamper.next() instead of our home-grown Clock.currentTimeMillis() in production for the local region cache.

This is what was here before recent changes.

SimpleTimestamper was introduced at some point as a part of Hibernate SPI:

/**
 * Generates increasing identifiers (in a single VM only). Not valid across multiple VMs.  Identifiers are not
 * necessarily strictly increasing, but usually are.
 * <p/>
 * Core while loop implemented by Alex Snaps - EHCache project - under ASL 2.0
 *
 * @author Hibernate team
 * @author Alex Snaps
 */
public final class SimpleTimestamper {
    private static final int BIN_DIGITS = 12;
    private static final AtomicLong VALUE = new AtomicLong();

    public static final short ONE_MS = 1 << BIN_DIGITS;

    public static long next() {
        while (true) {
            long base = System.currentTimeMillis() << BIN_DIGITS;
            long maxValue = base + ONE_MS - 1;

            for (long current = VALUE.get(), update = Math.max(base, current + 1); update < maxValue;
                 current = VALUE.get(), update = Math.max(base, current + 1)) {
                if (VALUE.compareAndSet(current, update)) {
                    return update;
                }
            }
        }
    }
    // ...
}

@pivovarit
Copy link
Contributor

pivovarit commented May 9, 2020

For the curious ones out there. A simple "benchmark " showing how drastic the difference can be:

        System.out.println(Stream.generate(() -> System.currentTimeMillis())
          .limit(1000)
          .distinct()
          .count()); // 3

        System.out.println(Stream.generate(() -> Clock.currentTimeMillis())
          .limit(1000)
          .distinct()
          .count()); // 4

        System.out.println(Stream.generate(() -> SimpleTimestamper.next())
          .limit(1000)
          .distinct()
          .count()); // 1000

@pivovarit pivovarit merged commit 1fd6789 into hazelcast:master May 18, 2020
@enozcan enozcan deleted the test_time_precision branch May 21, 2020 22:18
@pivovarit pivovarit added this to the 2.1.0 milestone Jun 19, 2020
@pivovarit pivovarit added the bug label Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants