> On July 13, 2016, 12:48 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsMonitor.java, > > line 63 > > <https://reviews.apache.org/r/49877/diff/1/?file=1441036#file1441036line63> > > > > [Info Question] If only supported in Linux, do you want to do a linux > > check by using System.getProperty("os.name")?
Now it'll be supported in both as we are using another method to get statistics. We rely on a more portable way using `ps` in the revised version of the patch. That works for both linux and mac. > On July 13, 2016, 12:48 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsMonitor.java, > > line 72 > > <https://reviews.apache.org/r/49877/diff/1/?file=1441036#file1441036line72> > > > > For the numbers matched, are we trying to match just valid numbers > > including negative numbers? If so, would it be better to use "-?[0-9]+" to > > be more specific? Also, if we are matching the whole, it is better to have > > "$" in the end since we are using "^" in the beginning. If fields get added at the end (of the procfs file which is fairly common), I believe using a `$` will cause a mismatch. None of these values I believe are negative. > On July 13, 2016, 12:48 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsMonitor.java, > > line 141 > > <https://reviews.apache.org/r/49877/diff/1/?file=1441036#file1441036line141> > > > > Is it possible that the file was not created due to other reasons, such > > as permission and etc.? If so, then this message might not be accurate. We're not creating this file, It's actually the 'procfs' mounted . man7.org/linux/man-pages/man5/proc.5.html > On July 13, 2016, 12:48 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/container/host/ProcfsBasedStatisticsMonitor.java, > > line 206 > > <https://reviews.apache.org/r/49877/diff/1/?file=1441036#file1441036line206> > > > > Do we want to log if match is not found? This might help us to capture > > the issue if the format is changed for the path somehow in the future. Thanks for the observation! Good find. I've logged it. (Although currently we rely on a different method to get statistics and this code path is not invoked..) - Jagadish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49877/#review141989 ----------------------------------------------------------- On July 12, 2016, 12:17 a.m., Jagadish Venkatraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49877/ > ----------------------------------------------------------- > > (Updated July 12, 2016, 12:17 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/host/ProcfsBasedStatisticsMonitor.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/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/TestProcfsBasedStatisticsMonitor.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 > >
