> 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.
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? - Sravya ----------------------------------------------------------- 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 > >
