-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Optimize the dirty dependency graph walk #27633
Optimize the dirty dependency graph walk #27633
Conversation
…alking its dependencies
@@ -282,28 +283,23 @@ private DependencyNode resolveRuntimeDeps(CollectRequest request) throws AppMode | |||
@Override | |||
public DependencyNode transformGraph(DependencyNode node, DependencyGraphTransformationContext context) | |||
throws RepositoryException { | |||
final Set<ArtifactKey> visited = new HashSet<>(); | |||
final Map<DependencyNode, DependencyNode> visited = new IdentityHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the Collections.newSetFromMap()
utility is sometimes also nice :-)
final Set<ArtifactKey> visited = Collections.newSetFromMap(new IdentityHashMap<>());
Failing Jobs - Building 067e888
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/opentelemetry/opentelemetry/deployment
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment integration-tests/micrometer-prometheus and 6 more 📦 extensions/opentelemetry/opentelemetry/deployment✖
|
@aloubyansky I think the failure is unrelated. @radcortez it might be related to the same issue with CONSUMER timing we had elsewhere? @aloubyansky I don't know if you want to change the Set/Map thing so I let you merge when you want. |
Maybe. I did some digging here #27368. And the puzzling part is that when the test fails, some spans have the same startTime, which messes with the expected order. That startTime is set by the OTel library, so we don't control that. I had a look and I didn't see anything out of the ordinary. I'm wondering if the server updates the SO time in the middle of the test and then it messes the spans, but it does seem a little far-fetched. |
The current implementation may appear to be hanging processing (very) dirty dependency graphs. This change checks whether a dependency node has been already processed before walking its dependencies.
Fix #27589