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.

Sign up for GitHub

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

Jump to bottom

Manage workload caches #79

Merged
merged 1 commit into from
Jan 25, 2019
Merged

Manage workload caches #79

merged 1 commit into from
Jan 25, 2019

Conversation

Mesbah-Alam
Copy link
Contributor

@Mesbah-Alam Mesbah-Alam commented Jan 24, 2019 •
edited
Loading

Run SharedClassesCacheChecker with only workload caches

Fixes: #78

Signed-off-by: [email protected] [email protected]

@Mesbah-Alam
Copy link
Contributor Author

Reviewer: @hangshao0

@hangshao0
Copy link
Contributor

Please add more details in your commit message.

@Mesbah-Alam
Copy link
Contributor Author

The test passes 5x Grinder on Internal Jenkins (Link shared with Hang on Slack).

@Mesbah-Alam
Copy link
Contributor Author

Mesbah-Alam commented Jan 24, 2019 •
edited
Loading

There is a failure in the Grinder run at Adopt: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/906/tapResults/

From 62.WL3.stderr log file:

JVMSHRC162E The wait for the creation mutex while opening shared memory has timed out
JVMSHRC662I Error recovery: destroyed semaphore set associated with shared class cache.
JVMJ9VM015W Initialization error for library j9shr29(11): JVMJ9VM009E J9VMDllMain failed
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
  • @hangshao0 any comment? Is it something related to the changes in this PR?

@Mesbah-Alam
Copy link
Contributor Author

Mesbah-Alam commented Jan 24, 2019 •
edited
Loading

5x Grinder at openj9 passes : https://ci.eclipse.org/openj9/view/Test/job/Test-Grinder/245/console

@Mesbah-Alam
Copy link
Contributor Author

Since the PR passes in internal Jenkins 5x Grinder and also OpenJ9 5x Grinder, it should be ready to merge.

@smlambert - Once you have a minute, please merge this PR.

@hangshao0
Copy link
Contributor

any comment? Is it something related to the changes in this PR?

No. I don't think it is related to this change.

smlambert
smlambert approved these changes Jan 24, 2019
View reviewed changes
Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

LGTM, noted some spelling / minor stuff

...dClasses.jvmti/src/test.sharedClasses.jvmti/net/openj9/sc/api/SharedClassesCacheChecker.java Outdated
// caches that might exist on the machine where the test is running). So, we should attempt
// to destroy only the test-related cache we are given.
if (info.getCacheName() == null) {
// We want to only delete the worklaod caches here.
Copy link
Contributor

Choose a reason for hiding this comment

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

workload vs worklaod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

...dClasses.jvmti/src/test.sharedClasses.jvmti/net/openj9/sc/api/SharedClassesCacheChecker.java Outdated
// We want to only delete the worklaod caches here.
// Any cache that might be listed in the default location - created
// or owned by other processes should be ignored.
// Cache created and used by the SharedClassesCacheCheceker process
Copy link
Contributor

Choose a reason for hiding this comment

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

SharedClassesCacheChecker vs SharedClassesCacheCheceker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

...dClasses.jvmti/src/test.sharedClasses.jvmti/net/openj9/sc/api/SharedClassesCacheChecker.java Outdated
@@ -195,7 +198,7 @@ boolean delete() {
rv = false;
break;
}
this.alreadySuccesfullyDeletedCaches.add(cacheName);
this.alreadySuccesfullyDeletedCaches.add(info.getCacheName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to switch to use info.getCacheName() on line 201, instead of cacheName defined up on line 163?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah.. Missed that. Updated!

@smlambert smlambert merged commit 385b4c2 into adoptium:master Jan 25, 2019
@Mesbah-Alam Mesbah-Alam mentioned this pull request Jan 25, 2019
Open
@Mesbah-Alam Mesbah-Alam deleted the managWorkloadCaches branch March 6, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlambert smlambert smlambert approved these changes

@hangshao0 hangshao0 hangshao0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mesbah-Alam @hangshao0 @smlambert