SophieTech88 commented on PR #985:
URL: https://github.com/apache/yunikorn-core/pull/985#issuecomment-2423578740
> To be honest, I don't really see this as an improvement. It makes the
tests themselves hard to follow.
Hi Craig,
Thank you so much for your feedback! I completely understand your concerns
regarding the readability of the combined tests.
I think this boils down to finding the right balance between readability and
avoiding code duplication. In the original code, the logic is certainly clearer
with separated test cases, but there is a lot of duplicate code due to the
similar steps involved in the preemption process. I believe this is why
@manirajv06 raised the issue in the first place—hoping to simplify the tests
and remove redundancy, while still preserving clarity. Please correct me if my
understanding is off!
As I reviewed the three test cases, I noticed that while their goals differ,
the preemption process itself is quite similar across all of them, which is
where the duplicated code comes from. Here's a breakdown of the common steps:
1. Setting up the two nodes and queues:
- The parameters for rootMaxRes, childQ2GuarRes, and app2Res are unique
to each test case to suit the specific scenario being tested.
- One idea I considered was passing all parameters (e.g., MaxRes and
GuaranteedRes for rootQ, parentQ, childQ1, childQ2) in a more structured way to
simplify things, but I felt that might lead to passing too many parameters.
2. Setting up app1 with its allocations: This is a common step across all
tests.
3. Setting up app2: Another similar step across tests.
4. Setting up headRoom, preemptor, and triggering preemption: This is where
the main difference lies between the tests:
- `TestTryPreemptionOnNode`: Preemption happens due to node resource
constraints.
- `TestTryPreemption_NodeWithCapacityLesserThanAsk`: No preemption
happens because the node's capacity is smaller than the request.
- `TestTryPreemptionOnQueue`: Preemption happens due to queue resource
constraints, which introduces an extra check for allocs.
5. Validating preemption results based on the test case.
While I tried to reduce the duplicated code, I realize there’s still a
challenge in balancing this with maintaining readability and clarity. The goal
was to make the code more concise without sacrificing logic transparency.
I've updated the code with a new approach that attempts to address these
trade-offs. I'm not sure if it fully aligns with your expectations, but I'd
really appreciate your thoughts on it. If you have any specific suggestions or
ideas, I'd be more than happy to incorporate them. I’m open to any feedback to
further improve it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
