> 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. > > Chris Pettitt wrote: > 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.
Totally missed the error calculation. I get it now, and I'm glad to see the behavior matches my initial expectation. > 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. > > Chris Pettitt wrote: > 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 :). Sounds good. This isn't a major issue. I just know that if I encountered this class in isolation, seeing the assertions in the class would help me reason about the expected inputs. - Jake ----------------------------------------------------------- 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 > >
