kingamarton commented on pull request #202: URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/202#issuecomment-721067250
Hi @yangwwei thank you for the review. I agree that the actual approach is hard to read and it is a kind of hack. I find your suggestion a dangerous one, because of the following reasons: - the client might configure some resource related predicates for the reservation list, what will always fail, because when the reservations are created it means that there aren't enough resources in the cluster - also the client might configure different predicates for the reservation that for the allocation, what also might cause unwanted behaviours. After analysing all this aspects I still think that the cleaner and safer solution is to expand the `GeneralPred` into the 4 predicates. I also checked the code again and the comment and the implementation of the generalPredicate function: `// GeneralPredicates checks whether noncriticalPredicates and EssentialPredicates pass` - I think we misunderstood this comment, because here it clearly says that both predicate types are checked. The rest of the comment is just a small explanation what this predicates are. I also checked the ordering, and there will be no difference either. So I will revert this last change set and keep the expanded predicate list. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
