dengpanyin commented on a change in pull request #961: SAMZA-2134:Enable table rate limiter by default. URL: https://github.com/apache/samza/pull/961#discussion_r268860718
########## File path: samza-api/src/main/java/org/apache/samza/table/descriptors/RemoteTableDescriptor.java ########## @@ -173,6 +179,41 @@ public RemoteTableDescriptor(String tableId) { return this; } + /** + * Disable both read and write rate limiter. If the read rate limiter is enabled, the user must provide a rate limiter + * by calling {@link #withRateLimiter(RateLimiter, TableRateLimiter.CreditFunction, TableRateLimiter.CreditFunction)} + * or {@link #withReadRateLimit(int)}. If the write rate limiter is enabled, the user must provide a rate limiter + * by calling {@link #withRateLimiter(RateLimiter, TableRateLimiter.CreditFunction, TableRateLimiter.CreditFunction)} + * or {@link #withWriteRateLimit(int)}. By default, both read and write rate limiters are enabled. + * + * @return this table descriptor instance. + */ + public RemoteTableDescriptor<K, V> withDisableRateLimiter() { + withDisableReadRateLimiter(); + withDisableWriteRateLimiter(); + return this; + } + + /** + * Disable the read rate limiter. + * + * @return this table descriptor instance. + */ + public RemoteTableDescriptor<K, V> withDisableReadRateLimiter() { + this.enableReadRateLimiter = false; + return this; + } + + /** + * Disable the write rate limiter. Review comment: @vjagadish1989 As mentioned earlier, we typically don't put optional field in the constructor. Also when talking to @weisong44 earlier, he recommended to have this API for the users. On the other hand, currently, we already have the following APIs to set rate limiter: (1) withRateLimiter(RateLimiter rateLimiter, TableRateLimiter.CreditFunction<K, V> readCreditFn, TableRateLimiter.CreditFunction<K, V> writeCreditFn) (2) withReadRateLimit(int creditsPerSec) (3) withWriteRateLimit(int creditsPerSec) If we further allow the user to config rate limiter from the constructor, the user may get confused if they have a rate limiter value in the constructor and a rate limiter configured using one of the above APIs. Also, the user have to give a magic number in order to disable the corresponding rate limiter. If the user config a very large value (not -1) with the intension to disable rate limiter, we still need to create a rate limiter using that value and apply the rate limiter towards read/write (wasting memory and CPU). Thinking all above, it seems cleaner to have an explicit API to disable rate limiters if the user has to. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services