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

Register Datadog classloader as parallel capable #448

Merged
merged 2 commits into from Aug 20, 2018

Conversation

realark
Copy link
Contributor

@realark realark commented Aug 17, 2018

  • Register Datadog classloader as parallel capable and add a test to assert instance is not locked on during classloading

@realark realark added this to the 0.13.0 milestone Aug 17, 2018
@realark realark requested a review from mar-kolya August 17, 2018 22:45
@Override
public Object getClassLoadingLock(String className) {
return super.getClassLoadingLock(className);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this method already had enough visibility to not needing this. If you remove this method tests still pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Looks like a protected method is visible to spock tests. Pretty useful for avoiding this problem of escalating exposure for testing. Removing this method.

boolean applicationDidNotDeadlock = true

then:
applicationDidNotDeadlock
Copy link
Contributor

Choose a reason for hiding this comment

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

Failure of this test would result in test runs just hanging forever. Would it be possible to implement this so test actually exits on failure? E.g. using two additional threads and making 'main' thread just wait reasonable amount of time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I added another thread and used spock's timeout annotation to fail this test after 1 minute.

@mar-kolya mar-kolya force-pushed the ark/parallel-dd-loader branch 2 times, most recently from a66df17 to 2d8102c Compare August 19, 2018 04:49
@realark realark dismissed mar-kolya’s stale review August 20, 2018 18:16

Added test timeout and removed classloading lock exposure. Please review again.

@realark realark merged commit 962af76 into master Aug 20, 2018
@realark realark deleted the ark/parallel-dd-loader branch August 20, 2018 23:46
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

2 participants