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

Make MigrationListener timers use wall-clock not CPU time [5.2.z] #25066

Merged
merged 3 commits into from Aug 1, 2023

Conversation

JackPGreen
Copy link
Contributor

Backport of #25028

The Javadoc on MigrationStats implied that the times recorded were wall-clock execution time, but the implementation was recording the execution time of each migration thread.

Resolved by deriving the migration times from the difference between the start of the migration (lastRepartitionTime) and completion time, rather than explicitly timing the operation in each thread.

Fixes HZ-2651

Checklist:

  • Labels (Team:, Type:, Source:, Module:) and Milestone set
  • Label Add to Release Notes or Not Release Notes content set
  • Request reviewers if possible
  • Send backports/forwardports if fix needs to be applied to past/future releases
  • New public APIs have @Nonnull/@Nullable annotations
  • New public APIs have @since tags in Javadoc

@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   PartitionMigrationListenerTest.testMigrationListenerTotalElapsedTime:456 With only one migration, totalElapsedMigrationTime (243,626,623) and elapsedMigrationTime (169,181,828) times should be equal expected:<243626623> but was:<169181828>
[INFO] 
[ERROR] Tests run: 50952, Failures: 1, Errors: 0, Skipped: 238
[INFO] 

[ERROR] There are test failures.

@JackPGreen JackPGreen closed this Jul 25, 2023
@JackPGreen JackPGreen reopened this Jul 25, 2023
@JackPGreen JackPGreen marked this pull request as draft July 25, 2023 14:44
@JackPGreen JackPGreen marked this pull request as ready for review July 28, 2023 10:26
@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file
---------ERRORS-----------
--------------------------
[ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder_2/hazelcast/src/main/java/com/hazelcast/internal/partition/impl/MigrationTimer.java:2: Line does not match expected header line of ' * Copyright (c) 2008-2022, Hazelcast, Inc. All Rights Reserved.'. [Header]
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.2.0:checkstyle (default) on project hazelcast: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.38 with /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder_2/checkstyle/checkstyle.xml ruleset. -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

@hz-devops-test
Copy link

The job Hazelcast-pr-compiler of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file
---------ERRORS-----------
--------------------------
[ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-compiler/hazelcast/src/main/java/com/hazelcast/internal/partition/impl/MigrationTimer.java:2: Line does not match expected header line of ' * Copyright (c) 2008-2022, Hazelcast, Inc. All Rights Reserved.'. [Header]
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.2.0:checkstyle (default) on project hazelcast: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.38 with /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-compiler/checkstyle/checkstyle.xml ruleset. -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

@JamesHazelcast JamesHazelcast added this to the 5.2.z milestone Jul 28, 2023
Copy link
Contributor

@JamesHazelcast JamesHazelcast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please update the description to state this PR also backports #25072 (I got a bit confused over this, especially as both commits/PRs have the same title)

@JackPGreen JackPGreen merged commit c675c68 into hazelcast:5.2.z Aug 1, 2023
8 checks passed
@JackPGreen JackPGreen deleted the backport-pr-25028-5.2.z branch August 1, 2023 17:04
devOpsHazelcast pushed a commit that referenced this pull request Feb 26, 2024
…2651][HZ-2939] (#793)

Backport of #25066 &
#25427

The Javadoc on `MigrationStats` implied that the times recorded were
wall-clock execution time, but the implementation was recording the
execution time of each migration thread.

Resolved by deriving the migration times from the difference between the
start of the migration (`lastRepartitionTime`) and completion time,
rather than explicitly timing the operation in each thread.

Fixes: [HZ-2651](https://hazelcast.atlassian.net/browse/HZ-2651),
[#25164](#25164) /
[HZ-2939](https://hazelcast.atlassian.net/browse/HZ-2939)

[HZ-2651]:
https://hazelcast.atlassian.net/browse/HZ-2651?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[HZ-2939]:
https://hazelcast.atlassian.net/browse/HZ-2939?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

GitOrigin-RevId: 3996061949a68d03857056d07c69e431f26d0038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants