[HUDI-2733] Add support for Thrift sync#4665
Conversation
stym06
changed the title
|
@nsivabalan can someone see if this looks good? |
nsivabalan
left a comment
nsivabalan
left a comment
There was a problem hiding this comment.
Can you confirm that you have tested this new hive sync mode and its working fine for all methods.
| public class ThriftDDLExecutor implements DDLExecutor { | ||
|
|
||
| private static final Logger LOG = LogManager.getLogger(ThriftDDLExecutor.class); | ||
| private final HMSClient client; |
There was a problem hiding this comment.
Can we name this HMSThriftClient. We already have hms mode and calling this HMSClient and not being used with "hms" mode doesn't sit well.
| } | ||
|
|
||
| @Override | ||
| public void close() { |
There was a problem hiding this comment.
this class for the most part has overlap with HMSDDLExecutor. Can we create a base class and inherit in both executors. you can introduce abstract methods just for the client specific calls. lets try to reuse code as much as possible.
There was a problem hiding this comment.
hey @nsivabalan can you suggest how I should be restructuring the classes? Thanks.
|
|
||
| String principal = conf.get(PRINCIPAL_KEY); | ||
| // TODO fixme | ||
| // String principal = null; |
There was a problem hiding this comment.
can we remove any residues from draft work.
| * @throws IOException if fails connecting to metastore | ||
| * @throws InterruptedException if interrupted during kerberos setup | ||
| */ | ||
| private void getClient(@Nullable URI uri) |
There was a problem hiding this comment.
can we fix method name. having "get" in the name, but method does not return anything.
|
|
||
| if (useSasl) { | ||
| LOG.debug("Using SASL authentication"); | ||
| HadoopThriftAuthBridge.Client authBridge = |
There was a problem hiding this comment.
I don't have good knowledge on these. But I assume you have tested this and hence gonna skim through.
| * Helper utilities. The Util class is just a placeholder for static methods, | ||
| * it should be never instantiated. | ||
| */ | ||
| public final class Util { |
There was a problem hiding this comment.
may be HiveSyncThriftModeUtils
|
@stym06 : let me know once the feedback is addressed. |
|
@stym06 : gentle ping. |
|
@nsivabalan apologies for the delayed response. let me check the comments. |
|
@stym06 any chance you can rebase and update this PR? |
4 participants
What is the purpose of the pull request
This PR adds support to sync Hudi metadata with Hive metastore using its Thrift client
Verify this pull request
(Please pick either of the following options)
This pull request is a trivial rework / code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.