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 depth limit rather than timeout on NamedOperationResolver #3149

Closed
tb06904 opened this issue Jan 24, 2024 · 1 comment · Fixed by #3174
Closed

Use depth limit rather than timeout on NamedOperationResolver #3149

tb06904 opened this issue Jan 24, 2024 · 1 comment · Fixed by #3174
Assignees
Labels
enhancement Improvement to existing functionality/feature
Milestone

Comments

@tb06904
Copy link
Member

tb06904 commented Jan 24, 2024

The NamedOperationResolver uses a timeout to control how long it will try resolve from cache, this is to stop infinite loops due to nested operations.

A better approach for this would be to implement a depth limit instead that controls how nested into a NamedOperation the resolver will go. This approach prevents infinite loops but also ensures the resolver succeeds under more circumstances and won't get stuck or timeout before finishing.

Originally posted by @GCHQDeveloper314 in #3148 (comment)

@tb06904 tb06904 changed the title Use depth limit rather than timeout on NamedOperationResolverake more sense here. Use depth limit rather than timeout on NamedOperationResolver Jan 24, 2024
@GCHQDeveloper314 GCHQDeveloper314 added this to the Backlog milestone Jan 24, 2024
@GCHQDeveloper314 GCHQDeveloper314 added the enhancement Improvement to existing functionality/feature label Jan 24, 2024
@GCHQDeveloper314
Copy link
Member

In addition to this, there's a problem with the logs created by this code. Every time it runs the following is logged:

graph.hook.NamedOperationResolver INFO  - Starting ResolverTask with timeout: 1MINUTES
graph.hook.NamedOperationResolver INFO  - finished ResolverTask

These shouldn't be at INFO level.

@tb06904 tb06904 modified the milestones: Backlog, v2.2.0 Mar 14, 2024
@tb06904 tb06904 self-assigned this Mar 14, 2024
@tb06904 tb06904 linked a pull request Mar 18, 2024 that will close this issue
tb06904 added a commit that referenced this issue Mar 20, 2024
…epth-limit' into gh-3149-namedoperationresolver-depth-limit
GCHQDeveloper314 added a commit that referenced this issue Mar 21, 2024
* add depth limit to name op resolver

* Tweak functionality for resolver

* update and add limit test

* update instance check to operation chains

* throw exception if any unresolved named ops

* add test for configurable limit

---------

Co-authored-by: rj77259 <141829236+rj77259@users.noreply.github.com>
Co-authored-by: GCHQDeveloper314 <94527357+GCHQDeveloper314@users.noreply.github.com>
GCHQDev404 added a commit that referenced this issue Mar 25, 2024
…tionresolver-depth-limit-test-change

# Conflicts:
#	core/graph/src/main/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolver.java
#	core/graph/src/test/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolverTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to existing functionality/feature
Projects
None yet
2 participants