Allow primary metastore to have prefix #161

AnanaMJ · 2019-04-17T09:22:16Z

Fixes #152

AnanaMJ · 2019-04-17T09:23:57Z

waggle-dance-api/src/main/java/com/hotels/bdp/waggledance/api/model/FederatedMetaStore.java

 import javax.validation.constraints.NotNull;

-import org.hibernate.validator.constraints.NotBlank;


Hibernate's @NotBlank is deprecated. This is what the docs say:

@deprecated use the standard {@link javax.validation.constraints.NotBlank} constraint instead

patduin · 2019-04-17T09:40:30Z

waggle-dance-api/src/main/java/com/hotels/bdp/waggledance/api/model/PrimaryMetaStore.java

-    return EMPTY_PREFIX;
+    String prefix = super.getDatabasePrefix();
+    if (prefix == null) {
+      prefix = "";


maybe we keep using EMPTY_PREFIX just so it is clear that it is a valid value

coveralls · 2019-04-17T09:40:31Z

Coverage increased (+0.02%) to 80.528% when pulling 2df1664 on primary-prefix into 1fadcb7 on master.

patduin · 2019-04-17T10:00:48Z

Could you add an WaggleDanceIntegrationTest test for this?

patduin · 2019-04-17T10:11:07Z

CHANGELOG.md

@@ -1,3 +1,7 @@
+## TBD
+### Changed
+* Allow primary metastore to have a prefix. See [#152](https://github.com/HotelsDotCom/waggle-dance/issues/152).


README also needs an update probably.

patduin · 2019-04-17T11:59:51Z

waggle-dance-integration-tests/src/main/java/com/hotels/bdp/waggledance/WaggleDanceRunner.java

+      checkArgument(isNotEmpty(name));
+      checkArgument(isNotEmpty(remoteMetaStoreUris));
+      primaryMetaStore = new PrimaryMetaStore(name, remoteMetaStoreUris, accessControlType, writableDatabaseWhiteList);
+      primaryMetaStore.setLatency(8000L);


line 182-185 can maybe be replaced with a call to primary(name, remoteMetaStoreUris......)
Or maybe rename/refactor this method into:

Builder withPrimaryPrefix(String prefix) { primaryMetaStore.setDatabasePrefix(prefix); return this; }

Ah, thank you! That's a great suggestion :)
I've just added it

patduin

This looks good to me

…ifferent

abhimanyugupta07

Looks good.

max-jacobs

👍

AnanaMJ · 2019-04-18T09:42:55Z

I’ve updated the PR with a change for get_all_functions in FederatedHMSHandler and FederatedHMSHandlerTest. Do you mind taking a look at it in case there's anything that I've missed?

patduin · 2019-04-18T10:56:19Z

waggle-dance-core/src/test/java/com/hotels/bdp/waggledance/server/FederatedHMSHandlerTest.java

@@ -767,12 +768,42 @@ public void flushCache() throws TException {

  @Test
  public void get_all_functions() throws MetaException, TException {


remove this test it is not a good test anymore.

patduin · 2019-04-18T10:56:26Z

waggle-dance-core/src/test/java/com/hotels/bdp/waggledance/server/FederatedHMSHandlerTest.java

    GetAllFunctionsResponse response = new GetAllFunctionsResponse();
    when(primaryClient.get_all_functions()).thenReturn(response);
    GetAllFunctionsResponse result = handler.get_all_functions();
    assertThat(result, is(response));
  }

+  @Test
+  public void prefixedPrimary_get_all_functions() throws TException {


rename to get_all_functions The default is that all functions will have the DB transformed so that's what we should test.

Ok, done :D

patduin · 2019-04-19T12:05:56Z

Could you make sure to test this with a view. Where the view references db's in the primary metastore and they get resolved correctly?

AnanaMJ · 2019-04-23T13:56:12Z

I tested this with a few views, and if they don't use functions, they work as expected. If they do use functions, an error like FAILED: SemanticException [Error 10011]: Invalid function <database>.<function> comes up. Just like the one in the issue you created (#165)

massdosage

Looks good, just one tiny documentation change and then I'm happy to approve.

massdosage · 2019-04-23T13:51:43Z

README.md

@@ -141,7 +141,7 @@ The table below describes all the available configuration values for Waggle Danc
 | `primary-meta-store`                                    | No       | Primary MetaStore config. Can be empty but it is advised to configure it. |
 | `primary-meta-store.remote-meta-store-uris`             | Yes      | Thrift URIs of the federated read-only metastore. |
 | `primary-meta-store.name`                               | Yes      | Database name that uniquely identifies this metastore. Used internally. Cannot be empty. |
-| `primary-meta-store.database-prefix`                    | No       | This will be ignored for the primary metastore and an empty string will always be used instead. |
+| `primary-meta-store.database-prefix`                    | No       | Prefix used to access this particular metastore and differentiate databases in it from databases in another metastore. The default prefix (i.e. if this value isn't explicitly set) is empty string.|


this particular -> the primary

massdosage · 2019-04-23T13:52:41Z

README.md

@@ -356,7 +356,7 @@ database is encountered that is not prefixed then the primary metastore is used
      remote-meta-store-uris: thrift://primaryLocalMetastore:9083
    federated-meta-stores:
      - name: federated
-        prefix: waggle_prod_
+        database-prefix: waggle_prod_


Oh dear, was this wrong in our documentation all along?

AnanaMJ added 2 commits April 17, 2019 09:59

Remove overwrite for primary prefix

a21b901

Add changelog

71b7b7a

AnanaMJ commented Apr 17, 2019

View reviewed changes

AnanaMJ requested a review from a team April 17, 2019 09:24

patduin reviewed Apr 17, 2019

View reviewed changes

AnanaMJ added 2 commits April 17, 2019 11:43

Keep using EMPTY_PREFIX and change README

862393e

Add integration test

a88db3e

patduin reviewed Apr 17, 2019

View reviewed changes

Cleaner code

9c57d4e

patduin approved these changes Apr 17, 2019

View reviewed changes

Remove override of setDatabasePrefix because it doesn't do anything d…

c6584f5

…ifferent

abhimanyugupta07 approved these changes Apr 17, 2019

View reviewed changes

Use correct key for database-prefix in example config

6b10d08

max-jacobs approved these changes Apr 17, 2019

View reviewed changes

AnanaMJ added 2 commits April 18, 2019 09:52

Add prefix for functions if primary has prefix and tests

ed3ea22

Undo changes that are not relevant to the PR

eba268e

patduin reviewed Apr 18, 2019

View reviewed changes

Remove test that is not needed and rename the one with prefixed primary

15fb87a

massdosage suggested changes Apr 23, 2019

View reviewed changes

Add tiny documentation change

2df1664

massdosage approved these changes Apr 23, 2019

View reviewed changes

AnanaMJ merged commit 45a8ff3 into master Apr 23, 2019

AnanaMJ deleted the primary-prefix branch April 23, 2019 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow primary metastore to have prefix #161

Allow primary metastore to have prefix #161

AnanaMJ commented Apr 17, 2019

AnanaMJ Apr 17, 2019

patduin Apr 17, 2019

AnanaMJ Apr 17, 2019

coveralls commented Apr 17, 2019 •

edited

Loading

patduin commented Apr 17, 2019

patduin Apr 17, 2019

AnanaMJ Apr 17, 2019

patduin Apr 17, 2019 •

edited

Loading

AnanaMJ Apr 17, 2019

patduin left a comment

abhimanyugupta07 left a comment

max-jacobs left a comment

AnanaMJ commented Apr 18, 2019

patduin Apr 18, 2019

AnanaMJ Apr 18, 2019

patduin Apr 18, 2019 •

edited

Loading

AnanaMJ Apr 18, 2019

patduin commented Apr 19, 2019

AnanaMJ commented Apr 23, 2019

massdosage left a comment

massdosage Apr 23, 2019

AnanaMJ Apr 23, 2019

massdosage Apr 23, 2019

patduin Apr 23, 2019

AnanaMJ Apr 23, 2019

		import javax.validation.constraints.NotNull;

		import org.hibernate.validator.constraints.NotBlank;

		@@ -767,12 +768,42 @@ public void flushCache() throws TException {

		@Test
		public void get_all_functions() throws MetaException, TException {

Allow primary metastore to have prefix #161

Allow primary metastore to have prefix #161

Conversation

AnanaMJ commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 17, 2019 • edited Loading

patduin commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin left a comment

Choose a reason for hiding this comment

abhimanyugupta07 left a comment

Choose a reason for hiding this comment

max-jacobs left a comment

Choose a reason for hiding this comment

AnanaMJ commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin commented Apr 19, 2019

AnanaMJ commented Apr 23, 2019

massdosage left a 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

coveralls commented Apr 17, 2019 •

edited

Loading

patduin Apr 17, 2019 •

edited

Loading

patduin Apr 18, 2019 •

edited

Loading