aturoczy commented on code in PR #4912:
URL: https://github.com/apache/hive/pull/4912#discussion_r1412590427
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -1299,9 +1299,10 @@ public static enum ConfVars {
*/
@Deprecated
METASTORE_BATCH_RETRIEVE_MAX("hive.metastore.batch.retrieve.max", 300,
+ new RangeValidator(1, null),
"Maximum number of objects (tables/partitions) can be retrieved from
metastore in one batch. \n" +
"The higher the number, the less the number of round trips is needed
to the Hive metastore server, \n" +
- "but it may also cause higher memory requirement at the client side."),
+ "but it may also cause higher memory requirement at the client side.
Batch value should be > 0 "),
Review Comment:
I think in a help message the grater then is better than >
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionIterable.java:
##########
@@ -164,6 +164,9 @@ public PartitionIterable(Collection<Partition>
ptnsProvided) {
* a Hive object and a table object, and a partial partition spec.
*/
public PartitionIterable(IMetaStoreClient msc, Table table, int batch_size)
throws MetastoreException {
+ if (batch_size < 1) {
+ throw new MetastoreException("Batch Size for Partition Iterable should
be > 0");
Review Comment:
same here
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/PartitionIterable.java:
##########
@@ -156,6 +156,9 @@ public PartitionIterable(Hive db, Table table, Map<String,
String> partialPartit
*/
public PartitionIterable(Hive db, Table table, Map<String, String>
partialPartitionSpec,
int batchSize, boolean getColStats) throws
HiveException {
+ if (batchSize < 1) {
+ throw new HiveException("Batch Size for Partition Iterable should be >
0");
Review Comment:
Same here. I think the following exception message would be more expressive:
_"Invalid batch size for partition iterable. Please use a batch size greater
than 0"_
Even if you choose the previous one, please does not use CamelCases for
every words.
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/TestGetPartitionInBatches.java:
##########
@@ -256,4 +258,21 @@ public void testBatchingWhenException() throws Exception {
// In case any duplicate/incomplete list is given by
hive.getAllPartitionsInBatches, the below assertion will fail
assert(partNames.size() == 0);
}
+
+ @Test
+ public void testBatchingWhenBatchSizeIsZero() throws MetaException {
+ HiveMetaStoreClient spyMSC = spy(msc);
+ hive.setMSC(spyMSC);
+ int batchSize = 0;
+ try {
+ new PartitionIterable(hive, hive.getTable(dbName, tableName),
null, batchSize);
+ } catch (HiveException e) {
+ Assert.assertTrue(e.getMessage().contains("Batch Size for
Partition Iterable should be > 0"));
Review Comment:
In this case would need to change the error message.
**It is not belong to your PR:**
btw I feel we overuse the HiveException. I know it is bit out of the scope
of this but I have seen several PR the Author checked the message of the
exception which is an anti-pattern. Every exception should be clear about the
nature of the exception. The message just provides details for the developer.
It is not an end-user interface.
If you are agree (@ayushtkn, @deniskuzZ @abstractdog @zratkai, @veghlaci05 )
, we should have a discussion in the future of the exceptions use in the hive.
HiveException is oddly overused.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
