----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39116/#review101978 -----------------------------------------------------------
Thanks Lili for the patch! Some comments: - Can you add a test case where you create a thrift message greater than the set value and make sure we fail gracefully? - I thought about what would be a good value for max_size, the only thing I can think right now is the biggest message we expect. Which I think would come from HMS for the full update. In which case I wonder if we will hit the 100 MB limit. We need to do some more testing around this. - I see that you are using the default directly in some places where we do not have access to the conf object. This should be properly documented to make it clear it is not configurable to avoid confusion. - Sravya Tirukkovalur On Oct. 8, 2015, 6:34 a.m., Li Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39116/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2015, 6:34 a.m.) > > > Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > Set max message size for thrift messages > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > c727403 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 19b0b49 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java > b585773 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPathsFullDump.java > f74a75d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > 67a3574 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > ae0eec2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 1af7a8b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > bc35742 > > Diff: https://reviews.apache.org/r/39116/diff/ > > > Testing > ------- > > > Thanks, > > Li Li > >
