----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68975/#review209443 -----------------------------------------------------------
Thanks Laszlo! This is a big patch indeed. Comments below. Could you check that the test cases are covering all possible scenarios? Thanks, Peter ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1896 (patched) <https://reviews.apache.org/r/68975/#comment293858> What does this method do? Moving files and updating partition data if the partition is already exists? Javadoc would be good anyway. Follow-up idea: update partition data with alter_partitions (multiple updates with 1 HMS call)? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Line 1909 (original), 1939 (patched) <https://reviews.apache.org/r/68975/#comment293846> nit: maybe do not reformat these lines if they are not needed ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Line 1939 (original), 1961 (patched) <https://reviews.apache.org/r/68975/#comment293847> nit: maybe do not reformat these lines if they are not needed ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Line 1948 (original), 1970 (patched) <https://reviews.apache.org/r/68975/#comment293848> nit: maybe do not reformat these lines if they are not needed ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Line 1966 (original), 1988 (patched) <https://reviews.apache.org/r/68975/#comment293850> nit: maybe do not reformat these lines if they are not needed ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1970-1971 (original), 1992-1993 (patched) <https://reviews.apache.org/r/68975/#comment293851> nit: maybe do not reformat these lines if they are not needed ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1978-1979 (original), 2000-2001 (patched) <https://reviews.apache.org/r/68975/#comment293849> nit: maybe do not reformat these lines if they are not needed ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2102-2103 (patched) <https://reviews.apache.org/r/68975/#comment293852> Concerned about this. When we call Hive.loadPartition we call 2 methods there: - loadPartitionInternal - 1 snapshot - addPartitionToMetastore - 1 snapshot Are we sure that these calls are: - Lightweight - so we can happily call them twice - Return the same value in both ocassions even if some other transacion is finished during this time? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2080-2082 (original), 2126-2128 (patched) <https://reviews.apache.org/r/68975/#comment293853> Maybe surround this with isDebugEnabled? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2083-2084 (original), 2129-2130 (patched) <https://reviews.apache.org/r/68975/#comment293854> We do not use batching for adding partitions? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2492-2506 (patched) <https://reviews.apache.org/r/68975/#comment293856> Do we need this? Can't we use equals method of maps to compare instead? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2511 (patched) <https://reviews.apache.org/r/68975/#comment293857> Note: We still might fail with OOM if too many new partitions are there. We store in memory all of the new and old partition specifications. Was this the same before? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2604-2607 (patched) <https://reviews.apache.org/r/68975/#comment293859> Shouldn't we do this when uploading the files? I think the original intent was to show the progress of the loading of the partitions. We might want to keep this functionality. ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 2491-2492 (original), 2615 (patched) <https://reviews.apache.org/r/68975/#comment293862> nit: Please-please-please no unneccessary formatting changes... :) ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Line 2496 (original), 2619 (patched) <https://reviews.apache.org/r/68975/#comment293860> We need the PerfLogEnd for LOAD_DYNAMIC_PARTITIONS ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Line 2500 (original), 2621 (patched) <https://reviews.apache.org/r/68975/#comment293861> Why do we shallow the original exception. We should add as a root cause ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java Lines 910 (patched) <https://reviews.apache.org/r/68975/#comment293842> Do we need this as public? ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java Lines 952 (patched) <https://reviews.apache.org/r/68975/#comment293841> Do we need this as public? ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java Lines 1015 (patched) <https://reviews.apache.org/r/68975/#comment293844> It might be a good idea to add this to the javadoc of the method too, since this is a serious assumption. Maybe move the javadoc to the interface, or copy there too? Let's talk about this. ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java Lines 1020-1031 (patched) <https://reviews.apache.org/r/68975/#comment293845> This code path is repeated like 5 times in this file. Not this patch, but should not be a better way to handle this in a follow-up? - Peter Vary On okt. 10, 2018, 2:33 du, Laszlo Pinter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68975/ > ----------------------------------------------------------- > > (Updated okt. 10, 2018, 2:33 du) > > > Review request for hive and Peter Vary. > > > Repository: hive-git > > > Description > ------- > > HIVE-20661: Dynamic partitions loading calls add partition for every > partition 1-by-1 > > > Diffs > ----- > > > ql/src/java/org/apache/hadoop/hive/metastore/SynchronizedMetaStoreClient.java > 0ab77e84c6222d35bcec9ce95ed02014911ef144 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java > 4de038913a5c9a2c199f71702b8f70ca84d0856b > > ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java > a2b57fb646899c54b63be14a8cde9b8644a973aa > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > a2ec09f7157140fda751ab777649c698b3319a16 > > > Diff: https://reviews.apache.org/r/68975/diff/1/ > > > Testing > ------- > > > Thanks, > > Laszlo Pinter > >
