----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49877/#review142434 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsGetter.java (line 34) <https://reviews.apache.org/r/49877/#comment208002> Just curious, if the default is the more portable version of PosixCommandBasedStatisticsGetter, why do we need this Procfs based implementation? What additional function does it provide? If our current usage is just to get the physical memory usage and Posix implementation already provides the functionality, I would vote to remove this one, if it does not give additional function. samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java (line 46) <https://reviews.apache.org/r/49877/#comment208003> Why is this named as "ProcfsBased..."? Does it only work w/ Procfs based implementation? nit: Why use lower case name for log but use capital case name for thread factory? I thought that we use Camel style names for variables and only all-capital case names for constants of primitive data types. samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java (line 165) <https://reviews.apache.org/r/49877/#comment208011> Wouldn't this return false when inserting a non-existing listener? That will return: return null == Boolean.TRUE; samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java (line 172) <https://reviews.apache.org/r/49877/#comment208009> Suggestion: change it to StatisticsMonitorThreadFactory. samza-core/src/main/java/org/apache/samza/container/host/SystemStatistics.java (line 23) <https://reviews.apache.org/r/49877/#comment208012> It would be good to add some explanation regarding to what are the expected stats here, in addition to physical memory bytes. If the only current use case is memory, why don't we just name it as SystemMemoryStatics? samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 572) <https://reviews.apache.org/r/49877/#comment208013> Shouldn't we consider to make one monitor for all physical resource usage by the process now? I can imagine that network IO and CPU percentage would soon be added as additional stats as well. Does it make sense to fold all into a localResourceMonitor which contains diskSpaceMonitor and hostStatisticsMonitor? samza-core/src/test/java/org/apache/samza/container/host/TestStatisticsMonitorImpl.java (line 63) <https://reviews.apache.org/r/49877/#comment208014> Is there any test for PosixBasedStatisticsGetter? Since that is the default, we need to add this to unit test. - Yi Pan (Data Infrastructure) On July 14, 2016, 2:37 a.m., Jagadish Venkatraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49877/ > ----------------------------------------------------------- > > (Updated July 14, 2016, 2:37 a.m.) > > > Review request for samza, Boris Shkolnik, Chris Pettitt, Fred Ji, Jake Maes, > Yi Pan (Data Infrastructure), and Navina Ramesh. > > > Repository: samza > > > Description > ------- > > This feature introduces physical memory monitoring in SamzaContainer. > > Context: > Often memory used by the SamzaContainer process includes > A. JVM Heap memory: This is where all JVM variables live. > B. Native memory: This memory lives out of the JVM heap and is not visible to > the JVM. Examples include used by RocksDb, native libraries that user code > depends on etc. > > User jobs could be killed by Yarn if their total memory (A+B) exceeds the > configured maximum of yarn.container.memory.mb. > > Currently, while our existing metrics provide visibility into [A] via JMX, we > don't have visibility into [B]. (as it's totally external to the JVM). > > This feature uses Linux ProcFS to provide a complete view of the memory (both > A & B) to help Samza users understand memory better. (Schedulers like Apache > Yarn that require a holistic view of memory (A+B) also use ProcFS. For the > curious, here's the Yarn implementation - > http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/hadoop-yarn-common/0.23.1/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java > that inspired this idea) > > Scope: The scope of this RB is only to Linux distributions. (Mac based > implementation may be a separate change list.) > > > Diffs > ----- > > build.gradle ba4a9d14fe24e1ff170873920cd5eeef656955af > checkstyle/import-control.xml 325c38131047836dc8aedaea4187598ef3ba7666 > > samza-core/src/main/java/org/apache/samza/container/disk/PollingScanDiskSpaceMonitor.java > 50c85007123dd568ef90cf028af33a93a4470cb6 > > samza-core/src/main/java/org/apache/samza/container/host/PosixCommandBasedStatisticsGetter.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsGetter.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/host/SystemStatistics.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/host/SystemStatisticsGetter.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/host/SystemStatisticsMonitor.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 18c09224bbae959342daf9b2b7a7d971cc224f48 > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala > 2044ce01ffded8434e762d99355d5df43642c66b > > samza-core/src/test/java/org/apache/samza/container/host/TestStatisticsMonitorImpl.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/49877/diff/ > > > Testing > ------- > > 1. Unit tests with mock PROC-FS snapshots of processes > 2. Deployed actual jobs on my dev box. > 2.1 Obtained the operating system's view of the container memory using > 'ps' and other tools. > 2.2 Verified that the total memory reported by the monitor is the same as > the OS's view of memory[2.1] > 3. Tested on various Linux distributions I could find internally: > - RHEL release 6.4, 6.5, 6.6 (Santiago) > > > Thanks, > > Jagadish Venkatraman > >
