[GitHub] [yunikorn-core] surahman commented on pull request #412: [YUNIKORN-1213] Configurable Health Checker Interval

[GitBox <gi...@apache.org>]

surahman commented on PR #412:
URL: https://github.com/apache/yunikorn-core/pull/412#issuecomment-1131965103

   > 1. Instead of the following config
   > 
   > ```
   > healthcheck:
   >   enabled: %s
   >   interval: 99s
   > partitions:
   >   - name: default
   >     queues:
   >       - name: root
   > ```
   > 
   > what was discussed looks like the following:
   > 
   > ```
   > scheduler:
   >   healthcheck:
   >     enabled: true
   >     interval: 30s
   > partitions:
   >   - name: default
   >      queues:
   >         - name: root
   > ```
   > 
   > We want to include all "global" (non-partition-specific) configs under a single parent "scheduler", including "checksum". I suggest using another JIRA to track the changes to the "checksum", just to make the PR smaller.
   
   Thank you for pointing this out, it was a misunderstanding on my part 🤦🏼‍♂️.
   I have created a [YUNIKORN-1220](https://issues.apache.org/jira/browse/YUNIKORN-1220) for the `checksum`.
   
   > 2. Please create a JIRA to track the document changes.
   
   [YUNIKORN-1219](https://issues.apache.org/jira/browse/YUNIKORN-1219)
   
   > 3. Please make sure the test case includes both having healthcheck section configured and not having this section.
   
   I did not include a test case for this scenario because this test should cover the default condition:
   https://github.com/apache/yunikorn-core/blob/aacdb887fb5ff02441d5c577d23698086d711f28/pkg/scheduler/health_checker_test.go#L64-L76
   
    I can see the need for an explicit test to catch the case though and have added one.


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org