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

Use different tokens instead of forcing WD and all HMS to use the same delegatetoken in the kerberos environment #313

Merged
merged 9 commits into from
May 29, 2024
Merged

Use different tokens instead of forcing WD and all HMS to use the same delegatetoken in the kerberos environment #313

merged 9 commits into from
May 29, 2024

Conversation

flaming-archer
Copy link
Contributor

📝 Description

1.Use WD's own token.
2. Support HMS to use different tokens separately.
3. Refactored the delegatetoken section, greatly simplifying the code logic.
4. After extensive testing in our environment, it is OK.

🔗 Related Issues

#309

@flaming-archer flaming-archer mentioned this pull request Apr 12, 2024
Open
@flaming-archer
Copy link
Contributor Author

@patduin Could you review this or have someone else review it as well. We have been making this change for several months, and tested many times. I think it can really help many people.

@flaming-archer
Copy link
Contributor Author

The failed reason seems to have nothing to do with my pr
Error: Failed to execute goal org.sonatype.plugins:nexus-staging-maven-plugin:1.6.8:deploy (injected-nexus-deploy) on project waggle-dance-rpm: Failed to deploy artifacts: Could not transfer artifact com.hotels:waggle-dance-api:jar:javadoc:4.0.0-20240412.083012-2 from/to sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots): authentication failed for https://oss.sonatype.org/content/repositories/snapshots/com/hotels/waggle-dance-api/4.0.0-SNAPSHOT/waggle-dance-api-4.0.0-20240412.083012-2-javadoc.jar, status: 401 Unauthorized -> [Help 1]

@patduin
Copy link
Contributor

patduin commented Apr 12, 2024 via email

patduin
patduin requested changes Apr 16, 2024
View reviewed changes
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.

I cannot accept this PR as it is now. This needs some refactoring to separate kerberos code from the non kerberos paths and remove all the static class dependencies introduced.

@flaming-archer
Copy link
Contributor Author

I cannot accept this PR as it is now. This needs some refactoring to separate kerberos code from the non kerberos paths and remove all the static class dependencies introduced.

Wait for me for a while, I will make some changes and submit it again.

@flaming-archer
Copy link
Contributor Author

@patduin @jmnunezizu I have completed the modifications, please help me take a look

  1. The static part of the code has been replaced.
  2. Strategy model for SaslThriftMetastoreClientManager.

@flaming-archer
Copy link
Contributor Author

At the same time, I changed the code to not require manual kinit and can automatically renew tickets.

@flaming-archer
Copy link
Contributor Author

I think the failed test case getTableMeta and Could not transfer artifact are not related to me.

@flaming-archer
Copy link
Contributor Author

@patduin @jmnunezizu Could you help review it? This feature is really useful for users in the kerberos environment.

@jmnunezizu
Copy link

Hi @flaming-archer – we'll get to it as soon as we can. Sorry for the delay and thanks for the additional changes and contribution.

@flaming-archer
Copy link
Contributor Author

Hi @flaming-archer – we'll get to it as soon as we can. Sorry for the delay and thanks for the additional changes and contribution.

It's been a week now....

@flaming-archer
Copy link
Contributor Author

@patduin @jmnunezizu This feature is really great. Could you help me take a look.

patduin
patduin requested changes May 24, 2024
View reviewed changes
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.

just some cleanups but looks ok, thanks for patience while we found time to review.

jmnunezizu
jmnunezizu requested changes May 24, 2024
View reviewed changes
...re/src/main/java/com/hotels/bdp/waggledance/client/AbstractThriftMetastoreClientManager.java Outdated Show resolved Hide resolved
...re/src/main/java/com/hotels/bdp/waggledance/client/AbstractThriftMetastoreClientManager.java Outdated Show resolved Hide resolved
...re/src/main/java/com/hotels/bdp/waggledance/client/AbstractThriftMetastoreClientManager.java Show resolved Hide resolved
...re/src/main/java/com/hotels/bdp/waggledance/client/AbstractThriftMetastoreClientManager.java Show resolved Hide resolved
...re/src/main/java/com/hotels/bdp/waggledance/client/AbstractThriftMetastoreClientManager.java Show resolved Hide resolved
...e-core/src/main/java/com/hotels/bdp/waggledance/client/SaslThriftMetastoreClientManager.java Show resolved Hide resolved
...e-core/src/main/java/com/hotels/bdp/waggledance/client/SaslThriftMetastoreClientManager.java Show resolved Hide resolved
...dance-core/src/main/java/com/hotels/bdp/waggledance/client/ThriftMetastoreClientManager.java Outdated Show resolved Hide resolved
waggle-dance-core/src/main/java/com/hotels/bdp/waggledance/server/FederatedHMSHandler.java Show resolved Hide resolved
waggle-dance-core/src/main/java/com/hotels/bdp/waggledance/server/SaslServerWrapper.java Outdated Show resolved Hide resolved
@patduin
Copy link
Contributor

patduin commented May 27, 2024

I've merged the other hive3 PR please have a look at the conflicts, thank you.

@flaming-archer
Copy link
Contributor Author

I've merged the other hive3 PR please have a look at the conflicts, thank you.

Sorry, I only saw it now. I saw that PR, it was submitted by my colleague, and I know her changes. I will modify it together based on this. Wait for me for a moment.

yangyuxia and others added 4 commits May 28, 2024 22:33
@yangyuxia @patduin @flaming-archer
Add personalized configuration parameters for each metastore. (Expedi…
bcdd35e 
…aGroup#315)

* Add personalized configuration parameters for each metastore.

* Add personalized configuration parameters for each metastore

* Recover

* Update junit test

* Update Junit Test

* Update Junit Test

* Update Junit Test

* Format the code and update the readme

* Revert

* Update FederatedMetaStoreTest.java

* Update PrimaryMetaStoreTest.java

* Update AbstractMetaStore.java

using new HashMap so the generated Yaml doesn't generate an anchor (reference &id001)

* Update YamlFederatedMetaStoreStorageTest.java

fixing test

---------

Co-authored-by: yangyx <[email protected]>
Co-authored-by: Patrick Duin <[email protected]>
@patduin @flaming-archer
fixed import
d5a01a6
@flaming-archer
review modification
d053698
@flaming-archer
resovle conflicts
abcfc04 
# Conflicts:
#	README.md
#	waggle-dance-api/src/main/java/com/hotels/bdp/waggledance/api/model/AbstractMetaStore.java
#	waggle-dance-api/src/test/java/com/hotels/bdp/waggledance/api/model/FederatedMetaStoreTest.java
#	waggle-dance-api/src/test/java/com/hotels/bdp/waggledance/api/model/PrimaryMetaStoreTest.java
#	waggle-dance-core/src/test/java/com/hotels/bdp/waggledance/mapping/service/impl/YamlFederatedMetaStoreStorageTest.java
@flaming-archer
Copy link
Contributor Author

@patduin @jmnunezizu Modified, pls take a look.

jmnunezizu
jmnunezizu approved these changes May 28, 2024
View reviewed changes
Copy link

@jmnunezizu jmnunezizu left a comment

Choose a reason for hiding this comment

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

One small comment. Then I'll approve.

...re/src/main/java/com/hotels/bdp/waggledance/client/AbstractThriftMetastoreClientManager.java Show resolved Hide resolved
@flaming-archer
Copy link
Contributor Author

@patduin pls take a look at it.

patduin
patduin approved these changes May 29, 2024
View reviewed changes
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 e774cb1 into ExpediaGroup:hive-3.x May 29, 2024
0 of 2 checks passed
@flaming-archer flaming-archer mentioned this pull request Jun 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmnunezizu jmnunezizu jmnunezizu approved these changes

@patduin patduin patduin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flaming-archer @patduin @jmnunezizu @yangyuxia