Skip to content

Conversation

@szehon-ho
Copy link
Member

Noticed a lot of these errors while trying to debug timeouted Hive tests

@github-actions github-actions bot added the hive label Aug 6, 2021
@szehon-ho szehon-ho changed the title [hive] Fix some message typos in HiveCatalog: Matastore => Metastore Hive: Fix some message typos in HiveCatalog: Matastore => Metastore Aug 6, 2021
@szehon-ho szehon-ho requested a review from pvary August 6, 2021 23:25

} catch (TException e) {
throw new RuntimeException("Failed to create namespace " + namespace + " in Hive MataStore", e);
throw new RuntimeException("Failed to create namespace " + namespace + " in Hive MetaStore", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, how do Hive folks prefer to write it? MetaStore or Metastore?

Copy link
Member Author

@szehon-ho szehon-ho Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly always used Metastore and it's like that in most of the Hive docs, but I've seen the camel case a lot too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve also always used Metastore.

Looking at this Hive design doc, it’s also Metastore with no capital S. I think this makes the most sense as it’s not really two words imo (and several Hive docs confirm it as being one word): https://cwiki.apache.org/confluence/plugins/servlet/mobile?contentId=27362072#Design-Metastore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say Metastore. Capitalizing the S was to match the class name.

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. I’m +0, only because I really feel it’s Metastore as one word and not MetaStore as two words.

I’ll let others chime in on that, but a basic search of docs etc seem to bring up Metastore as one word much more often than I’ve seen “official” docs using the phrase as two words.


} catch (TException e) {
throw new RuntimeException("Failed to create namespace " + namespace + " in Hive MataStore", e);
throw new RuntimeException("Failed to create namespace " + namespace + " in Hive MetaStore", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve also always used Metastore.

Looking at this Hive design doc, it’s also Metastore with no capital S. I think this makes the most sense as it’s not really two words imo (and several Hive docs confirm it as being one word): https://cwiki.apache.org/confluence/plugins/servlet/mobile?contentId=27362072#Design-Metastore

Thread.currentThread().interrupt();
throw new RuntimeException(
"Interrupted in call to createDatabase(name) " + namespace + " in Hive MataStore", e);
"Interrupted in call to createDatabase(name) " + namespace + " in Hive MetaStore", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with Metastore and not MetaStore (which the former is correct according to a number of Hive and Cloudera docs), this also needs to be updated - plus several other locations :)

@szehon-ho
Copy link
Member Author

szehon-ho commented Aug 12, 2021

Yea I personally prefer Metastore as well due to Hive docs, I would guess the camel case comes originally from the Hive code itself (HiveMetaStore and HiveMetaStoreClient being the main classes..), so I didnt see the original casing as a problem.

Anyway, removed camel case.

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @szehon-ho!

@szehon-ho
Copy link
Member Author

Rebase to re-trigger test

@szehon-ho
Copy link
Member Author

Test failure seemed flaky, try to fix it in : #2989 , rebase to trigger again

@pvary pvary merged commit 0f97536 into apache:master Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants