You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2020/05/17 18:51:19 UTC

[GitHub] [mesos] Digital-Chaos opened a new pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Digital-Chaos opened a new pull request #365:
URL: https://github.com/apache/mesos/pull/365


   Fixed detection of available free memory and swap.
   Tested on FreeBSD-12.1-STABLE (sysutils/apache-mesos port)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] bmahler commented on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
bmahler commented on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-630503075


   @Digital-Chaos thanks for fixing this!
   
   Whoops, looks like we don't have any unit tests for os::memory()! Could you add some here to ensure this fix is covered by tests:
    https://github.com/apache/mesos/blob/1.9.0/3rdparty/stout/tests/os_tests.cpp


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] Digital-Chaos edited a comment on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
Digital-Chaos edited a comment on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-631178968


   Added a basic test for os::memory() as requested. I wasn't sure how to verify the adding of the invalid pages to the free pages, but at least there is more coverage than before!  :-)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] Digital-Chaos edited a comment on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
Digital-Chaos edited a comment on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-631178968


   Added a basic test for os::memory() as requested. I wasn't sure how to verify the adding of the inactive pages to the free pages, but at least there is more coverage than before!  :-)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] Digital-Chaos commented on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
Digital-Chaos commented on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-632641435


   > Is it because not all entries have mib constants that we can just fill in without lookup?
   
   Yes, I believe that is the case.
   
   The other way I have seen this done is to call sysctlnametomib() once in some sort of initialisation and hold the mib somewhere to use later in any calls to sysctl(). 
   e.g. https://github.com/hishamhm/htop/blob/master/freebsd/FreeBSDProcessList.c
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] Digital-Chaos commented on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
Digital-Chaos commented on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-631178968


   Added a basic test for os::memory as requested. I wasn't sure how to verify the summation of the invalid pages to the free pages, but at least there is more coverage than before!  :-)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] bmahler closed pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
bmahler closed pull request #365:
URL: https://github.com/apache/mesos/pull/365


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] bmahler commented on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
bmahler commented on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-631787330


   @Digital-Chaos Thanks! Looked through the sysctl related documentation and the code makes sense to me. I will ship this as is.
   
   However, I wonder why the code bothers with the lookup by name -> mib transformation and doesn't just do by name (slower) or by mib (faster) entirely:
   
   ```
   sysctlbyname("vm.swap_info.0", ...)
   sysctlbyname("vm.swap_info.1", ...)
   // until ENOENT
   
   // Or:
   int mib[3] = { CTL_VM, XXX, 0 }; // Oh.. I can't find something that looks like swap_info,
                                    // is there not a mib constant we can use for every name we can lookup?
   sysctl(...);
   ```
   
   (This applies to the earlier sysctlbyname calls in this function as well). Is it because not all entries have mib constants that we can just fill in without lookup?
   
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [mesos] Digital-Chaos edited a comment on pull request #365: Fix FreeBSD memory detection (MESOS-10104)

Posted by GitBox <gi...@apache.org>.
Digital-Chaos edited a comment on pull request #365:
URL: https://github.com/apache/mesos/pull/365#issuecomment-631178968


   Added a basic test for os::memory() as requested. I wasn't sure how to verify the summation of the invalid pages to the free pages, but at least there is more coverage than before!  :-)
   


----------------------------------------------------------------
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:
users@infra.apache.org