Skip to content

[ZEPPELIN-6770] Update HbaseInterpreter to a subclass of ShellInterpr… - #5613

Open
paul8263 wants to merge 1 commit into
apache:masterfrom
paul8263:ZEPPELIN-6770
Open

paul8263 wants to merge 1 commit into
apache:masterfrom
paul8263:ZEPPELIN-6770

Conversation

@paul8263

Copy link
Copy Markdown
Contributor

What is this PR for?

Refactor HbaseInterpreter to extend ShellInterpreter instead of reimplementing shell process management from scratch.

The previous implementation duplicated a lot of logic that ShellInterpreter already provides: writing each paragraph to a temp script file, constructing a DefaultExecutor with a PumpStreamHandler, managing an ExecuteWatchdog, implementing cancel() and a FIFO scheduler, and cleaning up temp files. This patch deletes all of that (about 150 lines) and reduces HbaseInterpreter to the HBase-specific part only: resolving HBASE_HOME and turning a paragraph into one hbase shell invocation.

The main motivation is Kerberos support. ShellInterpreter extends KerberosInterpreter, so by subclassing it the HBase interpreter now inherits the same, already-tested Kerberos handling for free: zeppelin.shell.auth.type=kerberos, zeppelin.shell.keytab.location, zeppelin.shell.principal, and the periodic re-kinit on ticket expiry. No HBase-specific Kerberos code is required, and the same approach can be reused by any other shell-backed interpreter.

What type of PR is it?

Improvement / Refactoring

Todos

What is the Jira issue?

[ZEPPELIN-6770]

How should this be tested?

This update has been covered by UTs.
./mvnw test -pl hbase -am

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes. The doc has already been updated.

* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* Licensed to the Apache Software Foundation (ASF) under one or more

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to change the license part here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @jongyoul ,
This license part has been restored.

Comment thread hbase/pom.xml
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<groupId>org.apache.zeppelin</groupId>
<artifactId>zeppelin-shell</artifactId>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@paul8263 I know it's the proper way to extend the existing feature but I wouldn't recommend to include another interpreter. It sometimes leads unexpected behaviors as we designed every interpreter as independent component. Could you please mimic and/or re-implement features you use only in this interpreter? I'm also planning to existing dependencies between interpreters in the following year.

This branch has not been deployed

No deployments
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.

2 participants