> 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. > > Li Li wrote: > 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. > > Sravya Tirukkovalur wrote: > For test case: > - Yes, please configure the value to a small number in your tests (like > 10s or 100s). So that it is easy to hit this case. > - Yes, by gracefuly I meant we should have tests which verify the > exception. Also, we should make sure client is still able to talk to service > after the exception (to make sure failure was handled well) > > Ideal message size: > - Yes, I think there is a chance we might hit this max size for the > initial full update of HMS. Would be good to run some scale tests and come up > with recommendations for setting this value based on Hive meta data size. > Please feel free to file a follow on jira. > - I am slightly concerned that max size is not truly configurable, as it > not in Thriftserializer. Can you file a follow on jira here? We should figure > out a way to make this configurable. May be create a serializer singleton at > startup by passing a max_size parameter?
Thanks, Sravya! Will submit a new patch with tests to verify the exception and service behavior. Just created a jira for configurable max message size in ThriftSerializer: https://issues.apache.org/jira/browse/SENTRY-909 , and a jira for Ideal max message size: https://issues.apache.org/jira/browse/SENTRY-910 - 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 > >
