> On Oct. 8, 2015, 11:12 p.m., Sravya Tirukkovalur wrote: > > 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.
Thanks Sravya for the reviews! I will update it according to your comments. Here I have some questions: - Can you add a test case where you create a thrift message greater than the set value and make sure we fail gracefully? I have tried to create a larger thrift msg, and it failed as the heap or GC overhead limit exceeded in my local machine. I guess that our precommit test environment may not have larger heap size as well, so I set the maxMessageSize for thrift protocol to a smaller one instead. Besides, what do you mean of "fail gracefully"? eg. In the ThriftSerializer.deserialize method, it will throw IOException with the msg: Error deserializing thrift object TPathsDump, and the cause: Length exceeded max allowed I wonder if this means fail gracefully as we have returned a correct exception msg, and we not make the system down. - 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. How should we test around this? Create a test cluster and manually test it via "curl http://$(hostname -f):8038" ? - 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. I find the only place without conf object is serialize and deserialize methods in ThriftSerializer. I wonder if it is fine to add a method comment like: Use default max thrift message size on top of this message. - Li ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39116/#review101978 ----------------------------------------------------------- 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 > >
