> On May 10, 2017, 6:08 p.m., Naveen Gangam wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 3520 (patched) > > <https://reviews.apache.org/r/59070/diff/1/?file=1711073#file1711073line3520> > > > > Do we really have to re-fetch it from the database? can we have the > > createAndPersist() return the value it just inserted?
It is possible that createAndPersist can fail in case of some other HMS instance trying to add a UUID at the same time. In such case we need to rollback and retrieve again. Restructured the code a bit to remove the unnecessary second call to retrieve the uuid. > On May 10, 2017, 6:08 p.m., Naveen Gangam wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 3536 (patched) > > <https://reviews.apache.org/r/59070/diff/1/?file=1711073#file1711073line3536> > > > > nit: Can we make this comment a bit more useful to help us debug > > issues? like the hostname of this HMS and perhaps a timestamp as well? Since the GUID is not specific to hostname running the metastore service, I am not sure if adding HMS hostname to the description makes sense. Added the timestamp which could be useful. > On May 10, 2017, 6:08 p.m., Naveen Gangam wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 3554 (patched) > > <https://reviews.apache.org/r/59070/diff/1/?file=1711073#file1711073line3554> > > > > Is there a chance of this returning large amounts of entries? (as this > > table grows over time or due to some abnormality). > > Should we consider using a predicate for the query? The query is expected to return a Collection of only one entry since there cannot be multiple rows with the property key 'guid' in the table by its design. > On May 10, 2017, 6:08 p.m., Naveen Gangam wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 3558 (patched) > > <https://reviews.apache.org/r/59070/diff/1/?file=1711073#file1711073line3558> > > > > Should the UUID be redacted from the log file? Does this pose a > > security vulnerability as these logs get distributed? As far as I understand there is no security risk in logging the UUID. But I can remove the log if you think that would be better. > On May 10, 2017, 6:08 p.m., Naveen Gangam wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 3562 (patched) > > <https://reviews.apache.org/r/59070/diff/1/?file=1711073#file1711073line3562> > > > > I thought the idea is to be able to have multiple rows for "guid", one > > of each HMS instances? I do not see the code calling this API for this > > feature, but once we have multiple rows of "guid" in the DB, this call will > > fail every time rendering this feature unusable. > > will be have a check at startup that there arent multiple rows? Feels > > its better to fail-fast at startup than to allow HMS to be in this state. > > Am I making sense? There cannot be multiple rows with guid property key in the table since there is a unique key constraint on that column. Since this is the UUID of database not HMS service, the idea is to have a single GUID even when there are multiple HMS instances as long as they are accessing the same database instance. > On May 10, 2017, 6:08 p.m., Naveen Gangam wrote: > > metastore/src/model/package.jdo > > Lines 985 (patched) > > <https://reviews.apache.org/r/59070/diff/1/?file=1711078#file1711078line985> > > > > Shouldnt this be "DESCRIPTION" to match the table column name in the > > schema? good catch. Will fix that - Vihang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59070/#review174514 ----------------------------------------------------------- On May 10, 2017, 7:41 p.m., Vihang Karajgaonkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59070/ > ----------------------------------------------------------- > > (Updated May 10, 2017, 7:41 p.m.) > > > Review request for hive, Ashutosh Chauhan, Naveen Gangam, Sergio Pena, and > Sahil Takiar. > > > Bugs: HIVE-16555 > https://issues.apache.org/jira/browse/HIVE-16555 > > > Repository: hive-git > > > Description > ------- > > HIVE-16555 : Add a new thrift API call for get_metastore_uuid > > > Diffs > ----- > > > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java > 88b9faf8394a59de39be55b2dd2315db7a8d5ab4 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestEmbeddedHiveMetaStore.java > bc00d11e2a1c9fd66b89f1ceca100aaafe43cfed > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java > b95c25ca00751629577e014801c3fb9f1a99bd70 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java > ef02968e22363d537f58b6054266bf9bc87033ae > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java > 29768c1d660aac937c0cd1fa15fb70b571007d14 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java > 4a46f7537f3ceb16c45010b88786907109fd1090 > metastore/if/hive_metastore.thrift ca6a0076d1fbee4b0d904c1bafcc056ab739e4c4 > metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h > ca71711e09de962d1cd2ee2ee72b3fcbbac228bc > metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp > 9042cdb265373cd25ee9050fb59f6547f4dfc669 > metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp > b4a2a926428d529cc88954552eb561041404877d > metastore/src/gen/thrift/gen-cpp/hive_metastore_types.h > c21ded144484f83cf59b989eada5506ed8e9ba3b > metastore/src/gen/thrift/gen-cpp/hive_metastore_types.cpp > e3725a543ec44ae46f7475156cae270b37b01196 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/MetastoreDBProperty.java > PRE-CREATION > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > 19151507cae1921a38028582ce00e34cd00585eb > metastore/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php > 4fb71839471a1a7b8b8ebd9573212c7d40e9f39d > metastore/src/gen/thrift/gen-php/metastore/Types.php > 74f0028c4124c729385eeee954537e4fdaf992eb > metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > f2a97997a4aaaf0ed07dc094ee8303717e01284d > metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 8ee84af14f87b23956b6bee268c0092e439c17e0 > metastore/src/gen/thrift/gen-py/hive_metastore/ttypes.py > f26cb5b185cf6ae4b79786e6911f1b052822011a > metastore/src/gen/thrift/gen-rb/hive_metastore_types.rb > f1aa9a6a9738d8a2d544c15aaa5c1348a6e2ce6c > metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb > 04e63f3a9b858d79bfa4883c36c2ccce69bf55c4 > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > cbcfc72ac73ccfe48dd9b57eb9722ae092e7094b > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 53f81188c1cda13f20bdf757391024e0c289d9f9 > metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 023a2893c3b046999981cccf8e7ff21227601f6b > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > a83e12e8f3e3a2f3149e4bbc09524998d0e8928f > metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java > c22a1db046814bf987de0df33b79e718b8fd6dc6 > metastore/src/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > 39b1676eb9d3c344a6e66f06f096f3a9fb1931ca > metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java > f6420f5b99fc49df8675afdddd9718871b6c01bb > > metastore/src/model/org/apache/hadoop/hive/metastore/model/MMetastoreDBProperties.java > PRE-CREATION > metastore/src/model/package.jdo 969e19912791b5f5a2b9c5fa4c43800310f5080c > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 3e3fd20de069503fcedacf60fa1df90279af26b2 > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > 91d8c2af67ae1e49f8d41f16d8eee361b3b2abf9 > > > Diff: https://reviews.apache.org/r/59070/diff/2/ > > > Testing > ------- > > > Thanks, > > Vihang Karajgaonkar > >
