[ https://issues.apache.org/jira/browse/HIVE-2093?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13022670#comment-13022670 ]
jirapos...@reviews.apache.org commented on HIVE-2093: ----------------------------------------------------- ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/566/#review517 ----------------------------------------------------------- trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java <https://reviews.apache.org/r/566/#comment1053> Spelling trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/566/#comment1058> Why not make this a boolean function? I understand that the convention in this class is to have functions that return 0, or something more than 0, but it looks like all of the other functions can be boolean valued as well. Returning an integer in this situation is confusing. trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/566/#comment1059> Same deal here. Can you change this to return a boolean value? trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java <https://reviews.apache.org/r/566/#comment1061> You can eliminate a couple lines of code by using foreach syntax instead of an explicit iterator. trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java <https://reviews.apache.org/r/566/#comment1062> I draw the line at nested ternary statements. Can you please use if/else instead? trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java <https://reviews.apache.org/r/566/#comment1063> Nice to have: move DATABASE to the beginning of the list. trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java <https://reviews.apache.org/r/566/#comment1064> Why do you need to initialize the lock manager here, but not above in analyzeLockDatabase()? trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g <https://reviews.apache.org/r/566/#comment1065> should be "unlock database statement" trunk/ql/src/test/queries/clientnegative/lockneg6.q <https://reviews.apache.org/r/566/#comment1066> Please add some comments explaining what it is you're trying to test so that I can quickly tell how this differs from lockneg5, lockneg7, etc, etc. Please also consider changing the name of the file to something more descriptive so that I don't even have to read the comments. trunk/ql/src/test/results/clientnegative/authorization_fail_create_db.q.out <https://reviews.apache.org/r/566/#comment1067> Please make this less ambiguous and easier to parse by capitalizing SHOW GRANT. trunk/ql/src/test/results/clientnegative/exim_15_part_nonpart.q.out <https://reviews.apache.org/r/566/#comment1068> Any chance I can get a look at closed-source-hive? ;) - Carl On 2011-04-19 19:25:22, Siying Dong wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/566/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-04-19 19:25:22) bq. bq. bq. Review request for hive, Yongqiang He and namit jain. bq. bq. bq. Summary bq. ------- bq. bq. Still need to change some old tests' outputs. bq. bq. bq. This addresses bug HIVE-2093. bq. https://issues.apache.org/jira/browse/HIVE-2093 bq. bq. bq. Diffs bq. ----- bq. bq. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/DDLWork.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/LockDatabaseDesc.java PRE-CREATION bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/ShowLocksDesc.java 1095164 bq. trunk/ql/src/java/org/apache/hadoop/hive/ql/plan/UnlockDatabaseDesc.java PRE-CREATION bq. trunk/ql/src/test/queries/clientnegative/authorization_fail_create_db.q PRE-CREATION bq. trunk/ql/src/test/queries/clientnegative/authorization_fail_drop_db.q PRE-CREATION bq. trunk/ql/src/test/queries/clientnegative/lockneg6.q PRE-CREATION bq. trunk/ql/src/test/queries/clientnegative/lockneg7.q PRE-CREATION bq. trunk/ql/src/test/queries/clientnegative/lockneg8.q PRE-CREATION bq. trunk/ql/src/test/queries/clientnegative/lockneg9.q PRE-CREATION bq. trunk/ql/src/test/queries/clientpositive/database.q 1095164 bq. trunk/ql/src/test/results/clientnegative/authorization_fail_create_db.q.out PRE-CREATION bq. trunk/ql/src/test/results/clientnegative/authorization_fail_drop_db.q.out PRE-CREATION bq. trunk/ql/src/test/results/clientnegative/database_create_already_exists.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/database_create_invalid_name.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/database_drop_does_not_exist.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/database_drop_not_empty.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_01_nonpart_over_loaded.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_02_all_part_over_overlap.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_03_nonpart_noncompat_colschema.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_04_nonpart_noncompat_colnumber.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_05_nonpart_noncompat_coltype.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_06_nonpart_noncompat_storage.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_07_nonpart_noncompat_ifof.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_08_nonpart_noncompat_serde.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_09_nonpart_noncompat_serdeparam.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_10_nonpart_noncompat_bucketing.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_11_nonpart_noncompat_sorting.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_13_nonnative_import.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_14_nonpart_part.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_15_part_nonpart.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_16_part_noncompat_schema.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_17_part_spec_underspec.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_18_part_spec_missing.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_19_external_over_existing.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_20_managed_location_over_existing.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_21_part_managed_external.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_23_import_exist_authfail.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_24_import_part_authfail.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/exim_25_import_nonexist_authfail.q.out 1095164 bq. trunk/ql/src/test/results/clientnegative/lockneg6.q.out PRE-CREATION bq. trunk/ql/src/test/results/clientnegative/lockneg7.q.out PRE-CREATION bq. trunk/ql/src/test/results/clientnegative/lockneg8.q.out PRE-CREATION bq. trunk/ql/src/test/results/clientnegative/lockneg9.q.out PRE-CREATION bq. trunk/ql/src/test/results/clientpositive/add_part_exist.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/alter1.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/alter2.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/alter3.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/alter4.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/authorization_5.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/database.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/database_properties.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_00_nonpart_empty.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_01_nonpart.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_02_00_part_empty.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_02_part.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_03_nonpart_over_compat.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_04_all_part.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_04_evolved_parts.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_05_some_part.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_06_one_part.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_07_all_part_over_nonoverlap.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_08_nonpart_rename.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_09_part_spec_nonoverlap.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_10_external_managed.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_11_managed_external.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_12_external_location.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_13_managed_location.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_14_managed_location_over_existing.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_15_external_part.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_16_part_external.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_17_part_managed.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_18_part_external.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_19_part_external_location.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_20_part_managed_location.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_22_import_exist_authsuccess.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_23_import_part_authsuccess.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/exim_24_import_nonexist_authsuccess.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/lock1.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/lock2.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/lock3.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/rename_column.q.out 1095164 bq. trunk/ql/src/test/results/clientpositive/show_tables.q.out 1095164 bq. bq. Diff: https://reviews.apache.org/r/566/diff bq. bq. bq. Testing bq. ------- bq. bq. bq. Thanks, bq. bq. Siying bq. bq. > create/drop database should populate inputs/outputs and check concurrency and > user permission > --------------------------------------------------------------------------------------------- > > Key: HIVE-2093 > URL: https://issues.apache.org/jira/browse/HIVE-2093 > Project: Hive > Issue Type: Bug > Components: Metastore, Security > Reporter: Namit Jain > Assignee: Siying Dong > Attachments: HIVE.2093.1.patch, HIVE.2093.2.patch, HIVE.2093.3.patch, > HIVE.2093.4.patch > > > concurrency and authorization are needed for create/drop table. Also to make > concurrency work, it's better to have LOCK/UNLOCK DATABASE and SHOW LOCKS > DATABASE -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira