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

Build #2

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Build #2

wants to merge 13 commits into from

Conversation

t-aretz
Copy link

@t-aretz t-aretz commented Oct 4, 2023

No description provided.

Copy link
Member

@Lehmann-Fabian Lehmann-Fabian left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. Please remove all the logs or change the log level.
Please address/answer the other remarks.
And document somewhere how to use this feature.

src/main/java/cws/k8s/scheduler/model/TaskInput.java Outdated Show resolved Hide resolved

// print Tasks
System.out.println("Tasks before Label Alignment");
unscheduledTasks.stream().map(obj -> obj.getConfig().getName()).forEach(System.out::println);
Copy link
Member

Choose a reason for hiding this comment

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

Do not log this in stable version

Copy link
Member

Choose a reason for hiding this comment

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

Now you use Stdout.
Do not log this at all, or log at debug level if you need

@@ -32,6 +34,8 @@
import java.util.List;
import java.util.Map;

import com.fasterxml.jackson.databind.ObjectMapper;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this unused import

Comment on lines +106 to +107
// ObjectMapper objetMapper = new ObjectMapper();
// Map<String,String> nodelabel = objectMapper.convertValue(config.additional.get("tasklabelconfig"),Map.class);
Copy link
Member

Choose a reason for hiding this comment

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

Remove this

Comment on lines -110 to -111
Prioritize prioritize;
NodeAssign assign;
Copy link
Member

Choose a reason for hiding this comment

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

You can undo this change.

Copy link
Member

Choose a reason for hiding this comment

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

Or make prioritize and assign configurable


// print Tasks
System.out.println("Tasks before Label Alignment");
unscheduledTasks.stream().map(obj -> obj.getConfig().getName()).forEach(System.out::println);
Copy link
Member

Choose a reason for hiding this comment

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

Now you use Stdout.
Do not log this at all, or log at debug level if you need

@@ -468,6 +468,7 @@ Map<NodeWithAlloc, Requirements> getAvailableByNode(){
}
logInfo.add("------------------------------------");
log.info(String.join("\n", logInfo));

Copy link
Member

Choose a reason for hiding this comment

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

I think you can leave this class as it was.

}

final PodWithAge pod = task.getPod();
// log.info("Pod: " + pod.getName() + " Requested Resources: " + pod.getRequest() );
Copy link
Member

Choose a reason for hiding this comment

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

Remove

Comment on lines +63 to +74
for ( Map.Entry<NodeWithAlloc, Requirements> e : availableByNode.entrySet() ) {
final NodeWithAlloc node = e.getKey();

if(nodeName.equals(node.getName())){
log.info("Aligned Pod to node: " + node.getName());
alignment.add( new NodeTaskAlignment( node, task ) );
availableByNode.get( node ).subFromThis(pod.getRequest());
log.info("--> " + node.getName());
task.getTraceRecord().foundAlignment();
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please request this by the key and do not iterate the Map

public class NodeLabelAssign extends Scheduler {

private final Prioritize prioritize;
private final NodeAssign nodeAssigner;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name this defaultNodeAssigner or fallbackNodeAssigner to clearify its function


// first alignemnt (LabelAssign)
List<NodeTaskAlignment> alignmentLabelAssign = nodeLabelAssigner.getTaskNodeAlignment(unscheduledTasks, availableByNode);
List<String> namesList = alignmentLabelAssign.stream().map(obj -> obj.task.getConfig().getName()).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

listOfUnassignedTasks

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to make variable alignmentLabelAssign from type NodeLabelAssign and then store a list of unscheduled tasks and fetch it here. Then you don't need to calculate this here again.

}
}

// second alignemnt (FairAssign)
Copy link
Member

Choose a reason for hiding this comment

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

alignemnt -> alignment
Please add a more informative description:
Second Alignment: Here, all nodes are assigned that do not have a label.
This is not necessarily a FairAssign (The constructor can be called with different strategies)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants