[YUNIKORN-3084] Fix Inconsistency in Allocation Removal from sortedRequests

[mitdesai]

What is this PR for?

The binary search in the remove method used the LessThan() comparison function, which compares allocations based on priority and creation time. The comparison should be by allocation key.

When multiple allocations have the same priority and creation time, it is possible that the binary search will find an allocation that is different than the one we want to remove.

What type of PR is it?

• - Bug Fix
• - Improvement
• - Feature
• - Documentation
• - Hot Fix
• - Refactoring

Todos

• - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-3084

How should this be tested?

Create a Job with 1000 executors. Delete all the executors. Atleast one of the executors will have issue getting removed.

Screenshots (if appropriate)

Questions:

• - The licenses files need update.
• - There is breaking changes for older versions.
• - It needs documentation.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.67%. Comparing base (bf04bd4) to head (bb1891b).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1022      +/-   ##
==========================================
- Coverage   82.69%   82.67%   -0.02%     
==========================================
  Files          98       98              
  Lines       15682    15683       +1     
==========================================
- Hits        12968    12966       -2     
- Misses       2439     2441       +2     
- Partials      275      276       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mitdesai]

I don't think the e2e test failure is due to this change.

[chenyulin0719]

Yeah... It looks like something is broken in the Gang Scheduling E2E test.

[chenyulin0719]

+1 LGTM.

A minor feedbcak, I think the second test TestRemoveWithSamePriorityAndTimeButDifferentInstances, might be a bit redundant. Maybe we can remove it? Does anyone else have any comments on it?

[mitdesai]

Thanks for the review @chenyulin0719 and @pbacsko. I was trying some else when I wrote that test. I will update the PR.

[mitdesai]

The test in question was not adding any additional value. So I have removed it.