Adding internal hive connector #68

zhljen · 2017-03-15T01:59:27Z

No description provided.

ajoymajumdar · 2017-03-15T23:40:00Z

...at-connector-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFactory.java

@@ -95,7 +99,11 @@ private void init() {
 private IMetacatHiveClient createLocalClient() throws MetaException {
 final HiveConf conf = getDefaultConf();
 configuration.forEach(conf::set);
- return new EmbeddedHiveClient(MetacatHMSHandler.newRetryingHMSHandler("metacat", conf, true));
+ DataSourceManager.get().load(catalogName, configuration);


We will have to rethink the use of DataSourceManager. For now, lets add a TODO item.

ajoymajumdar · 2017-03-15T23:43:07Z

...-connector-hive/src/main/java/com/netflix/metacat/connector/hive/util/HivePartitionName.java

@@ -0,0 +1,45 @@
+/*


Why do we need this class? We can use QualifiedName.

ajoymajumdar · 2017-03-15T23:43:51Z

metacat-connector-hive/src/main/java/com/netflix/metacat/connector/hive/util/HiveTableName.java

@@ -0,0 +1,45 @@
+/*


Why do we need this class? We can use QualifiedName.

ajoymajumdar · 2017-03-15T23:44:59Z

metacat-connector-hive/src/main/java/com/netflix/metacat/connector/hive/util/RetryHelper.java

@@ -0,0 +1,227 @@
+/*


We should use https://github.com/rholder/guava-retrying.

ajoymajumdar · 2017-03-15T23:46:40Z

How are we handling sorting and pagination in the embedded hive client?

They are handled at the service layer.

tgianos · 2017-03-15T23:41:53Z

...ctor-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorDatabaseService.java

@@ -108,6 +110,24 @@ public void delete(@Nonnull final ConnectorContext requestContext, @Nonnull fina
 * {@inheritDoc}.
 */
 @Override
+ public void update(@Nonnull final ConnectorContext context, @Nonnull final DatabaseInfo databaseInfo) {


I'd recommend adding Lombok @NonNull to these fields so your code is precondition checked to avoid NPE

tgianos · 2017-03-15T23:43:30Z

...ctor-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorDatabaseService.java

@@ -108,6 +110,24 @@ public void delete(@Nonnull final ConnectorContext requestContext, @Nonnull fina
 * {@inheritDoc}.
 */
 @Override
+ public void update(@Nonnull final ConnectorContext context, @Nonnull final DatabaseInfo databaseInfo) {
+ try {
+ this.metacatHiveClient.alterDatabase(databaseInfo.getName().getDatabaseName(),


You call getName() a lot in here. Might be better to save it to a local variable. The compiler probably optimizes this to squash the function call but if we ever change the implementation of getName to do some processing under the hood you would replicate executing that functionality multiple times this way. If you put it in a local variable you only have to do it one time. good overall advice

tgianos · 2017-03-15T23:45:10Z

...at-connector-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFactory.java

@@ -75,17 +79,17 @@ public HiveConnectorFactory(final String name, final Map<String, String> configu
 private void init() {


I'm not sure I understand the purpose of an init() method that is only called from the constructor anyway. Why not just do the logic in the constructor? I've seen @ajoymajumdar do this as well so he might have a reason

tgianos · 2017-03-15T23:47:07Z

...at-connector-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFactory.java

 }
- final Module hiveModule = new HiveConnectorModule(name, configuration, infoConverter, client);
+ final Module hiveModule = new HiveConnectorModule(catalogName, configuration, infoConverter, client);
 final Injector injector = Guice.createInjector(hiveModule);
 this.databaseService = injector.getInstance(ConnectorDatabaseService.class);


While this is probably fine cause we know the inner workings of the module/injector and you're just going to return a singleton it's probably a bad practice in general because if the injector really returns a different instance every time with different logic (e.g. a provider method) then users wouldn't get new updated versions of the database service every time. They'd be stuck with the old one.

tgianos · 2017-03-15T23:47:55Z

...at-connector-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFactory.java

@@ -106,15 +114,17 @@ private static HiveConf getDefaultConf() {
 result.setInt("javax.jdo.option.DatastoreWriteTimeoutMillis", 60000);


I'd make all these strings constants with names as well as their default values you're setting here. That way we can quickly change them in one place if we have to use them in multiple places.

tgianos · 2017-03-15T23:48:59Z

...at-connector-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFactory.java

 }

 @Override
 public void stop() {
 try {
 client.shutdown();
 } catch (TException e) {
- log.warn(String.format("Failed shutting down the catalog: %s", name), e);
+ log.warn(String.format("Failed shutting down the catalog: %s", catalogName), e);


You don't have to embed a String.format inside the slf4j logs if you're just doing string replacement. It's redundant. you could change this to be "Failed shutting down the catalog: {}", catalogName, e and it would be the same.

tgianos · 2017-03-15T23:51:34Z

...n/java/com/netflix/metacat/connector/hive/client/embedded/HivePersistenceManagerFactory.java

+ final DataSource dataSource = DataSourceManager.get().get(name);
+ final String jdoTimeout = JDO_TIMEOUT.get();
+ final Map<String, Object> properties = Maps.newHashMap();
+ properties.put("datanucleus.autoCreateSchema", props.get("datanucleus.autoCreateSchema"));


These should all probably be constants.

tgianos · 2017-03-15T23:52:27Z

...tor-hive/src/main/java/com/netflix/metacat/connector/hive/client/embedded/HivePrivilege.java

+ * @return set of hive privilege
+ */
+ public static Set<HivePrivilege> parsePrivilege(final PrivilegeGrantInfo userGrant) {
+ final String name = userGrant.getPrivilege().toUpperCase(java.util.Locale.ENGLISH);


I don't quite understand this switch statement. It seems you can either have 1 or all but no other permutations?

* package name refactor * package name refactor * adding embedded client * fixing the functionTest * fixing check style * removing the port setting in function test * fixing the thrif functional test with embedded hive store * fixing checkstyple * removing commented code in functionalTest * fixing the hardcode string to constants * removing unused property * removing the guava-retrying in dependencies

ajoymajumdar reviewed Mar 15, 2017

View reviewed changes

tgianos suggested changes Mar 15, 2017

View reviewed changes

Zhen Li added 10 commits March 15, 2017 18:15

package name refactor

da08d1d

package name refactor

d090e72

adding embedded client

15e9218

fixing the functionTest

37c8668

fixing check style

62b48cb

removing the port setting in function test

07147b9

fixing the thrif functional test with embedded hive store

7c8dff6

fixing checkstyple

481c9df

removing commented code in functionalTest

7b777de

fixing the hardcode string to constants

9ee295a

zhljen force-pushed the canonical-types branch from c16936d to 9ee295a Compare March 16, 2017 17:08

Zhen Li added 2 commits March 16, 2017 10:09

removing unused property

0da2608

removing the guava-retrying in dependencies

1117a95

ajoymajumdar approved these changes Mar 16, 2017

View reviewed changes

tgianos approved these changes Mar 16, 2017

View reviewed changes

tgianos merged commit dc9dbce into Netflix:canonical-types Mar 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding internal hive connector #68

Adding internal hive connector #68

zhljen commented Mar 15, 2017

ajoymajumdar Mar 15, 2017

ajoymajumdar Mar 15, 2017 •

edited

Loading

ajoymajumdar Mar 15, 2017

ajoymajumdar Mar 15, 2017

ajoymajumdar commented Mar 15, 2017 •

edited by zhljen

Loading

tgianos Mar 15, 2017

tgianos Mar 15, 2017

tgianos Mar 15, 2017

tgianos Mar 15, 2017

tgianos Mar 15, 2017

tgianos Mar 15, 2017

tgianos Mar 15, 2017

tgianos Mar 15, 2017

		@@ -75,17 +79,17 @@ public HiveConnectorFactory(final String name, final Map<String, String> configu
		private void init() {

		@@ -106,15 +114,17 @@ private static HiveConf getDefaultConf() {
		result.setInt("javax.jdo.option.DatastoreWriteTimeoutMillis", 60000);

Adding internal hive connector #68

Adding internal hive connector #68

Conversation

zhljen commented Mar 15, 2017

Choose a reason for hiding this comment

ajoymajumdar Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajoymajumdar commented Mar 15, 2017 • edited by zhljen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajoymajumdar Mar 15, 2017 •

edited

Loading

ajoymajumdar commented Mar 15, 2017 •

edited by zhljen

Loading