> On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsGetter.java, > > line 34 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443721#file1443721line34> > > > > 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.
My original motivation was - if we decide to parse ProcFs to get some useful statistic later, we atleast have the source code to do it (that has already been unit-tested). I don't have a strong opinion on this one but Since, you've voiced your preference to delete it (maintainablity), I've nuked it :) > On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java, > > line 46 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443722#file1443722line46> > > > > 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. Good find, I've fixed it. I've made 'LOG' upper case. > On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java, > > line 165 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443722#file1443722line165> > > > > Wouldn't this return false when inserting a non-existing listener? That > > will return: > > return null == Boolean.TRUE; Fixed in the new iteration! Thanks for the careful review. I also ended up adding an unit test after your comment. :) > On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java, > > line 172 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443722#file1443722line172> > > > > Suggestion: change it to StatisticsMonitorThreadFactory. Thanks for the suggestion! I've fixed it > On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/test/java/org/apache/samza/container/host/TestStatisticsMonitorImpl.java, > > line 63 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443728#file1443728line63> > > > > Is there any test for PosixBasedStatisticsGetter? Since that is the > > default, we need to add this to unit test. Added unit tests for PosixBasedGetter, The checks verify if the value of the memory bytes is positive. (We could also have an upperbound on the result but I felt it may be excessive.) Happy to add more checks if you think so. > On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/host/SystemStatistics.java, > > line 23 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443723#file1443723line23> > > > > 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? My thought was that other statistics include cpu, network, and any other statistic exposed by the OS at a per-process level. Since, the current implementation is only memory, I've renamed it to SystemMemoryStats. > On July 15, 2016, 9:34 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > line 572 > > <https://reviews.apache.org/r/49877/diff/3/?file=1443726#file1443726line572> > > > > 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? Discussed this offline. Since, the reporting cadence and APIs are different, we decided that we can come up with an unified API later (if it makes sense). - Jagadish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49877/#review142434 ----------------------------------------------------------- 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 > >
