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.

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

Add hive hook configuration parameters for each hms #322

Merged
merged 7 commits into from
Jul 30, 2024

Conversation

yangyuxia
Copy link

📝 Description

When configuring hive hooks in waggle-dance-server.yml, it will take effect on all primary hms or federate hms. It does not support specifying hive hooks for individual hms.
Now it is supported to add hive hook configuration for a single primary hms or federate hms in waggle-dance-federation.yml without affecting each other.

🔗 Related Issues

#320

Copy link
Contributor

@patduin patduin left a comment

Choose a reason for hiding this comment

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

Nice! could you add a unit test please.

@@ -133,13 +139,36 @@ public void unreachableMetastoreClient() {
}
}

@Test
public void loadMetastoreFilterHook(){

Choose a reason for hiding this comment

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

Why is this empty?

Copy link

@czzczzczz9898 czzczzczz9898 left a comment

Choose a reason for hiding this comment

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

nice feature to deal with same path issue

Copy link
Contributor

@flaming-archer flaming-archer left a comment

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


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


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.

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

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

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");

Copy link
Contributor

@patduin patduin left a comment

Choose a reason for hiding this comment

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

sorry for late feedback I was out for a while, please have a look, just minor comments.

Copy link
Contributor

@patduin patduin left a comment

Choose a reason for hiding this comment

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

Thank you!

@patduin patduin merged commit bb3861f into ExpediaGroup:hive-3.x Jul 30, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants