Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add hive hook configuration parameters for each hms #322

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Add hive hook configuration parameters for each hms #322

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
62678a8
Add hive hook configuration parameters for each hms
Jul 4, 2024
5c69dfe
Added unit tests for hive hooks that work on a single hms
Jul 5, 2024
61609db
Clean up the excess unit test
Jul 8, 2024
ab44690
Add hive hook custom config unit test
Jul 10, 2024
0d67f26
Rename variable name
Jul 10, 2024
1ff4846
Remove try catch and organize imports
Jul 29, 2024
9508e99
Set StorageDescriptor location
Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...e/src/main/java/com/hotels/bdp/waggledance/mapping/model/MetaStoreMappingFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ private MetaStoreFilterHook loadMetastoreFilterHook(AbstractMetaStore metaStore)
conf.set(property.getKey(), property.getValue());
}
}
Map<String, String> metaConfigurationProperties = metaStore.getConfigurationProperties();
if (metaConfigurationProperties != null) {
for (Map.Entry<String, String> property : metaConfigurationProperties.entrySet()) {
conf.set(property.getKey(), property.getValue());
}
}
conf.set(HiveConf.ConfVars.METASTORE_FILTER_HOOK.varname, metaStoreFilterHook);
try {
Class<? extends MetaStoreFilterHook> filterHookClass = conf
Expand Down
37 changes: 33 additions & 4 deletions ...c/test/java/com/hotels/bdp/waggledance/mapping/model/MetaStoreMappingFactoryImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,28 @@
*/
package com.hotels.bdp.waggledance.mapping.model;

import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.CoreMatchers.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

organize imports please

import static org.hamcrest.MatcherAssert.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import static com.hotels.bdp.waggledance.api.model.AbstractMetaStore.newFederatedInstance;

import java.io.File;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;

import org.apache.hadoop.hive.metastore.DefaultMetaStoreFilterHookImpl;
import org.apache.hadoop.hive.metastore.MetaStoreFilterHook;
import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
import org.apache.hadoop.hive.metastore.api.Table;
import org.apache.thrift.TException;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.Mockito;
Expand Down Expand Up @@ -64,6 +69,7 @@ public class MetaStoreMappingFactoryImplTest {
new WaggleDanceConfiguration(), new SplitTrafficMetastoreClientFactory());

private MetaStoreMappingFactoryImpl factory;
public @Rule TemporaryFolder temporaryFolder = new TemporaryFolder();

@Before
public void init() {
Expand Down Expand Up @@ -133,13 +139,36 @@ public void unreachableMetastoreClient() {
}
}

@Test
public void loadMetastoreFilterHook(){

Copy link

Choose a reason for hiding this comment

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

Why is this empty?

}

@Test
public void loadMetastoreFilterHookFromConfig() {
AbstractMetaStore federatedMetaStore = newFederatedInstance("fed1", thrift.getThriftConnectionUri());
federatedMetaStore.setHiveMetastoreFilterHook(PrefixingMetastoreFilter.class.getName());
Map<String,String> metaStoreConfigurationProperties = new HashMap<>();
metaStoreConfigurationProperties.put(PrefixingMetastoreFilter.PREFIX_KEY,"prefix-test-");
federatedMetaStore.setConfigurationProperties(metaStoreConfigurationProperties);

MetaStoreMapping mapping = factory.newInstance(federatedMetaStore);
assertThat(mapping, is(notNullValue()));
assertThat(mapping.getMetastoreFilter(), instanceOf(PrefixingMetastoreFilter.class));

MetaStoreFilterHook filterHook = mapping.getMetastoreFilter();
assertThat(filterHook, instanceOf(PrefixingMetastoreFilter.class));

Table table = new Table();
try {
StorageDescriptor sd = new StorageDescriptor();
File localWarehouseUri = temporaryFolder.newFolder("local-warehouse");
sd.setLocation(new File(localWarehouseUri,"local_database/local_table").toURI().toString());
Copy link
Contributor

@patduin patduin Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please try if a simple string also works, maybe we don't need to create a dummy folder just for string manupilations.:
sd.setLocation("file:///tmp/local_database/local_table");

table.setSd(sd);

String oldLocation=sd.getLocation();
assertThat(filterHook.filterTable(table).getSd().getLocation(), equalTo("prefix-test-" + oldLocation ) );
}catch (Exception e){
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove try catch. No point in swallowing exceptions in test best to just throw it and let it fail the test.

}
}

@Test
Expand Down
7 changes: 5 additions & 2 deletions ...core/src/test/java/com/hotels/bdp/waggledance/mapping/model/PrefixingMetastoreFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@
* */
public class PrefixingMetastoreFilter implements MetaStoreFilterHook {

public static final String PREFIX = "prefix-";
public static final String PREFIX_KEY = "hive.metastore.hooks.prefix";
public static final String PREFIX_DEFAULT = "prefix-";
private final String prefix;

public PrefixingMetastoreFilter(HiveConf conf) {
prefix = conf.get(PREFIX_KEY,PREFIX_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is PREFIX_KEY meaningless? Is it ok just use hardcode to determine if it is the same as "prefix -".

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of ab44690 and 0d67f26, I think it's okay

}

@Override
Expand Down Expand Up @@ -140,7 +143,7 @@ private void setLocationPrefix(Partition partition) {

private void setLocationPrefix(StorageDescriptor sd) {
String location = sd.getLocation();
sd.setLocation(PREFIX + location);
sd.setLocation(prefix + location);
}

}
Loading