Proposed Changes QSB Frameworks #2
Conversation
There was a problem hiding this comment.
@kiranprakash154 - Can you limit this PR to non-renaming changes? Hard to review with all the stuff together
| static final ParseField RESOURCE_VALUE_FIELD = new ParseField("value"); | ||
| static final ParseField RESOURCE_TYPE_FIELD = new ParseField("resourceType"); | ||
| static final ParseField THRESHOLD_FIELD = new ParseField("threshold"); | ||
| static final ParseField THRESHOLD_IN_LONG_FIELD = new ParseField("thresholdInLong"); |
There was a problem hiding this comment.
Why do we need this parameter ?
| SandboxServiceSettings.MAX_RESOURCE_LIMIT_GROUP_COUNT, | ||
| SandboxServiceSettings.NODE_LEVEL_REJECTION_THRESHOLD, | ||
| SandboxServiceSettings.NODE_LEVEL_CANCELLATION_THRESHOLD |
There was a problem hiding this comment.
We need to rename these to RESOURCE specific settings
| */ | ||
|
|
||
| package org.opensearch.search.sandboxing; | ||
|
|
There was a problem hiding this comment.
Can we add javadocs for these new classes wherever applicable ?
|
|
||
| import java.util.Comparator; | ||
|
|
||
| public class LongestRunningTaskFirst extends AbstractTaskCancellationStrategy { |
There was a problem hiding this comment.
I think we should move these strategy classes into a package.
| import java.util.Comparator; | ||
|
|
||
| public class MostResourceIntensiveTaskFirst extends AbstractTaskCancellationStrategy { | ||
| private final SandboxResourceType resourceType; |
There was a problem hiding this comment.
We might have multiple resources in the future so we should keep this flexible and open to add more resource types.
| public SandboxModule() {} | ||
|
|
||
| @Override | ||
| protected void configure() { |
There was a problem hiding this comment.
This is no longer the case as we should be binding the interfaces to different implementations as discussed.
| // Accumulate resource usage for each task in the sandbox | ||
| for (Task task : tasks) { | ||
| for (SandboxResourceType resourceType: TRACKED_RESOURCES) { | ||
| sandboxUsage.put(resourceType, resourceType.getResourceUsage(task)); |
There was a problem hiding this comment.
This logic is incorrect and the sandbox usage for a sandbox will only hold the resource usage for the last visited task.
|
|
||
| import java.util.Map; | ||
|
|
||
| public class ResourceUsageData { |
There was a problem hiding this comment.
Do we really need this class ? I would rather have this as a member variable of all the data related to sandbox.
|
|
||
| import org.opensearch.tasks.Task; | ||
|
|
||
| public class TaskData { |
There was a problem hiding this comment.
I think we should move the data from this class and ResourceUsageData into CancellationContext instead of having multiple layers of indirection. SInce we are adding this code in core hence should minimise adding unnecessary construct.
You can still review this even though it is in draft. You can ignore the merge conflicts for now, the files and their changes are still visible, the conflicts are mainly due to renaming of ResourceLimitGroup to sandbox.
Description
This draft PR proposes changes to Kaushal's PR - opensearch-project#13311
Changes include :
Intention of the PR -
Get feedback from @kaushalmahi12 & @jainankitk and incorporate them.
Frameworks related PR depends on the PR Kaushal will soon create to make Sandbox (or RLG) and its schema available in main.