> On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > The code is super clean and well tested! > > > > Mostly conceptual questions below. > > > > Also, I think we need an entry in the config table for the new configs: > > docs/learn/documentation/versioned/jobs/configuration-table.html
Thanks Jake. All of your feedback is super helpful. I would love to get your take on a few of my responses below. General take away: more documentation around config and some of the subtler pieces of logic would be helpful. > On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaEnforcer.java, > > line 64 > > <https://reviews.apache.org/r/48080/diff/1/?file=1402147#file1402147line64> > > > > This comment suggests the quota is per-task, but the config structure > > "container.disk.quota.bytes" suggests it is per-container. Perhaps the term > > "task" is overloaded and I'm misinterpreting it. Nope. Good call. Will fix this to say container. > On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaEnforcer.java, > > line 72 > > <https://reviews.apache.org/r/48080/diff/1/?file=1402147#file1402147line72> > > > > Just an observation. This could be implemented as a Segment Tree > > https://en.wikipedia.org/wiki/Segment_tree True - a segment tree would be a clear win if we were managing a larger number of intervals. I think the approach here is simpler (less LoC anyway) and I'm expecting a small number of ranges - e.g. 2 or 3 at most. Otherwise reasoning about it is going to be difficult. > On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaPolicy.java, > > lines 23-24 > > <https://reviews.apache.org/r/48080/diff/1/?file=1402148#file1402148line23> > > > > Is this the "percentage of available disk space" on the entire disk, or > > the percentage available to the Task? percentage of quota. I'll expand on the doc to make this clearer. > On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaPolicy.java, > > line 28 > > <https://reviews.apache.org/r/48080/diff/1/?file=1402148#file1402148line28> > > > > I'm not sure I see the vision. It seems like this patch is expecting > > users to configure a set of policies with non overlapping watermarks and > > the DiskQuotaEnforcer relies on this paradigm. > > > > Maybe because this patch doesn't add a config table update describing > > how to compose a set of policies to achieve a desired behavior, but I found > > it difficult to reason about how this works without drawing it out on my > > notepad. I had to explore the code to find details like: > > 1. Whether the watermark values correspond to diskQuotaUtilization or > > diskQuotaRemainingPercentage > > 2. Whether the watermarks are allowed to overlap > > 3. If the watermarks overlap, is the workFactor additive, > > multiplicative, or precedence? > > > > To me though, the simplest representation of a throttling policy is a > > simple function: > > workfactor = policy.apply(quotaUtilizationPct) > > > > This, as an API, would support both user-defined policy functions or an > > encapsulated implementation of this watermark system. Sorry you had difficulty working through this. That is a clear indication that the documentation is insufficient. It helps to have a fresh pair of eyes after looking at this on and off for a while. I do want to have clear docs on this that we can eventually move to the config table, but I don't want to publish to the config table until we're "GA" with the featue. To address your questions: 1. Percentages are disk quota remaining. 2. Watermarks ranges can overlap, but not in a way that would lead to ambiguity. If a sample drops below one or more low water marks then we take the work rate of the lowest water mark. If a sample rises above one or more high water marks then we set the work rate to the policy immediately above the maximum exceeded policy. This means that one range cannot entirely contain or equal another. I'd be happy to restrict to non-overlapping ranges entirely if it makes things clearer and we don't see an operational need for them. 3. None, there is no ambiguity about which rule to apply. Having an interface for mapping remaining quota to work factor is reasonable. BTW, I would also like to get Jon to review the configs as he is going to be the main consumer. > On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaPolicy.java, > > lines 34-35 > > <https://reviews.apache.org/r/48080/diff/1/?file=1402148#file1402148line34> > > > > Should there be any verification that lowWaterMarkPercent <= > > highWaterMarkPercent and that both of them should be values betweeen 0.0 > > and 1.0? > > > > Actually, I found the former verification in DiskQuotaEnforcer, but > > expected to find it here. I don't see any verification of the min watermark. Yes, we should verify the lower bound. I put the check in the enforcer because I wanted to keep all of the checks in one place, but I can see your argument for moving them here. I'll do the upper / lower bounds and their relation checks here and do the overlapping range checks in the enforcer. Will also add some additional tests :). > On June 2, 2016, 6:24 p.m., Jake Maes wrote: > > samza-core/src/main/java/org/apache/samza/util/ThrottlingExecutor.java, > > line 70 > > <https://reviews.apache.org/r/48080/diff/1/?file=1402152#file1402152line70> > > > > Won't this continue to add more delay if the workFactor stays constant? > > So it becomes an rapid decay instead of a static throttle value for each > > policy? > > > > It seems the job would essentially halt after a small number of loops > > for any factor other than an infintesimally small one. > > > > Also, this behavior seems to overlap with the multiple policies. When I > > first saw the multiple policies, I thought, "ok, so to implement a linear > > decay, I'd specify a set of policies, each one with a tighter throttle" I > > was assuming the throttle rate was static. But seeing this additive code, I > > can't imagine using multiple policies because the job will rapidly slow > > with even just one. Sorry, this logic must be a bit to clever - at least a comment in the code is warranted. General back story: I would love to drop the whole pendingNanos thing and just say "a little error is not going to kill anything". The reason pendingNanos exists is that the best precision the JVM can muster for sleep is 1ms. So we need to accumulate enough to get that first sleep. We're adding to the pendingNanos here, but we are reducing them a few lines down in the sleep call. The sleep call is returning the amount of error on the sleep (which may be negative). Previously I actually measured the error here instead of in the sleep call and did the decrement inline. I'm not wed to doing it in the sleep call - I moved it there because it made the HighResClock a little nicer for other potential use cases that would need to do the same. I'd be interested in your perspective on one of these or an alternative approach. Given the confusion, at least a comment in the code is warranted. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48080/#review135892 ----------------------------------------------------------- On May 31, 2016, 5:27 p.m., Chris Pettitt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48080/ > ----------------------------------------------------------- > > (Updated May 31, 2016, 5:27 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > This change introduces a ThrottlingExecutor which is used to control the > rate of execution in the main run loop. The DiskQuotaEnforcer houses the > rules for switching from one DiskQuotaPolicy to the next as new disk > usage samples arrive. > > By default, no throttling will occur. New policies can be added using > the following form: > > ``` > container.disk.quota.bytes=XXX > container.disk.quota.policy.count=2 > container.disk.quota.policy.0.lowWaterMark=0.4 > container.disk.quota.policy.0.highWaterMark=0.5 > container.disk.quota.policy.0.workFactor=0.5 > container.disk.quota.policy.1.lowWaterMark=0.05 > container.disk.quota.policy.1.highWaterMark=0.1 > container.disk.quota.policy.1.workFactor=0.05 > ``` > > See ThrottlingExecutor for details about how the work factor works and > DiskQuotaPolicy for details about how the low and high water mark > configuration work. > > > Diffs > ----- > > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaEnforcer.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/disk/DiskQuotaPolicy.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/disk/DiskSpaceMonitor.java > 2a565be7858a4d3a6adbc49989b43b71ca3f6721 > samza-core/src/main/java/org/apache/samza/util/HighResolutionClock.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/util/SystemHighResolutionClock.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/util/ThrottlingExecutor.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala > 3f25eca6e3dffc57360e8bd8c435177c2a9a910a > > samza-core/src/main/scala/org/apache/samza/container/SameThreadExecutor.scala > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > cf3c4c0ab08a59760bc899c6f2027755e933b350 > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala > 9e6641c3628290dc05e1eb5537e86bff9d37f92c > samza-core/src/main/scala/org/apache/samza/util/Util.scala > fc3d085d7fff9f7dcec766ba48e550eb0052e99d > > samza-core/src/test/java/org/apache/samza/container/disk/TestDiskQuotaEnforcer.java > PRE-CREATION > samza-core/src/test/java/org/apache/samza/util/TestThrottlingExecutor.java > PRE-CREATION > samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala > 05b4e5c37578340eefe6d412f5a76f540bec6fa6 > > Diff: https://reviews.apache.org/r/48080/diff/ > > > Testing > ------- > > - Added new unit tests > - Ran existing tests with gradle test > - Verified throttling behavior and instrumentation with local deployment > - Verified average latency impact of feature to be < 150ns for Linux and OSX > > > Thanks, > > Chris Pettitt > >
