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

Allow presto to connect to kerberized Hive clusters (v2) #4576

Closed
wants to merge 16 commits into from

Conversation

losipiuk
Copy link
Contributor

No description provided.

@losipiuk
Copy link
Contributor Author

I checked connectivity to Hive metastore with SASL over socks proxy and it works.

return UserGroupInformation.getCurrentUser();
}
catch (IOException e) {
throw Throwables.propagate(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be categorized

Copy link
Member

Choose a reason for hiding this comment

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

IOException could be thrown only for tocken authentication from Credentials.readTokenStorageFile. As we are not using that type of authentication i don't think that it make sense to add special exception code for that particular issue.

Add proxy UserGroupInformation cache to impersonating implementations of
HadoopAuthentication. If no caching was used and new proxy UGI was
created on each call to getUserGroupInformation(String) HDFS Filesystem
mechanism did not work. Different proxy UGI are not equal in terms of
equals() method even if they represent same user. For this reason
new entry was created in HDFS Filesystem cache for each new proxy user.
This patch allows configuring where temporary files are created in hdfs during
INSERT/CREATE TABLE AS SELECT flow via hive connector.
New configuration property 'hive.hdfs.temporary.directory' was added. If
%USER% appears in value of property it is replaced with id of user
currently executing query.
@arhimondr
Copy link
Member

@electrum rebased onto actual master + comments addressed.

@dain dain self-assigned this Mar 2, 2016
@losipiuk
Copy link
Contributor Author

losipiuk commented Mar 2, 2016

When we benchmarked the solution we noticed great performance degradation when authenticating wrappers are in use. Simple queries on ORC table were executing 3x slower than without wrappers.

Solution 1

We have a working solution for that but it is somewhat hacky. It depends on patching UserGroupInformation in hadoop library.

These are PR for 3 versions of hadoop lib:

The source of performance degradation is fact that UserGroupInformation.doAs(PrivilegedAction) is a delegate to Subject.doAs(PrivilegedAction) from java.security, which is slow.

The change we made exploits the fact that call to actual Subject.doAs is rarely needed, when compared to number of method calls we wrap into UGI.doAs(PrivilegedAction) in Presto.
We also depend on the fact, that when call to Subject.doAs(PrivilegedAction) is actually needed, th Hadoop lib rewraps the code which needs that again into UGI.doAs(PrivilegedAction).

The change we made is basically extending UserGroupInformation class with faster versions of doAs methods taking (SubjectPrivilegedAction and SubjectPrivilegedExceptionAction).
We also change the original doAs(PrivilegedAction) and doAs(PrivilegedExceptionAction) and getCurrentUser() to interact with logged-user context set by the faster method versions.

In Presto's authenticating wrappers we use faster versions of UGI.doAs methods.

See this commit for actual diff: prestodb/presto-hadoop-apache2@240a81d

This is not a clean solution, as it patches external library and depends on brittle code flow assumptions.
It is very effective in terms of performance though.

Solution 2

After implementing this we also got another idea. It does not involve patching Hadoop lib but have other drawbacks.

Let's focus on ConnectorPageSource wrapper, as this is culprit for read-flow. The solution has two parts:

  1. Use UGI.doAs wrappers only in getNextPage() and close(). This assumes that other methods do not need authentication. This is probably true for most implementation but generally it does not have to be the case. We can extend the mechanism so ConnectorPageSource implementation can somehow declare (through annotation?) which methods need to use authentication.
  2. the wrapper for getNextPage() has buffer of pages and uses UGI.doAs only when buffer of pages is empty. The buffer is filled with multiple pages (order of 50) within single UGI.doAs wrap block. This way we amortize the cost of single doAs over multiple calls to CPS.getNextPage().

Similar mechanism can be implemented for ConnectorPageSink. RecordCursor is problematic and probably we would have to skip special implementation of RecordPageSource with this approach.

The drawback of this approach is that we are using more memory (buffer of pages), and we process data in less streamlined manner.
Also in case when other methods than getNextPage actually need authorization we have a problem again.

@electrum, @dain. What do you think about implemented solution? We would really appreciate input here. Any idea how to solve the problem here in a cleaner way would be very beneficial.

cc: @arhimondr, @pnowojski, @ilfrin

@dain
Copy link
Contributor

dain commented Mar 7, 2016

the wrapper for getNextPage() has buffer of pages and uses UGI.doAs only when buffer of pages is empty. The buffer is filled with multiple pages (order of 50) within single UGI.doAs wrap block. This way we amortize the cost of single doAs over multiple calls to CPS.getNextPage().

This won't work because we return lazy blocks. With ORC, we need to know which column values are needed before the next page is fetched, because when fetching a page we advance all streams.

In general, I think the solution to this is that we pass the UGI to the HivePageSourceFactory or HiveRecordCursorProvider during construction and require the implementation to handle the UGI. Except for a few obscure file formats, the file format code opens a file input stream during creation and never goes back to the file system to open new streams. We can then add special handling for these formats as we find them. (BTW this is the same approach we take with the thread context class loader). With this approach we would remove the UGI setting wrappers for page source and record cursor.

private UserGroupInformation getCurrentHdfsUser()
{
try {
return UserGroupInformation.getCurrentUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

The UGI should be explicitly passed in during the construction, so we should not need thread tricks here.

@dain
Copy link
Contributor

dain commented Mar 23, 2016

IIRC, when we talked, we decided that we were going to do more work on this. Let me know when that is done and I'll take another look.

@arhimondr
Copy link
Member

@dain Yes, that is right.

Superseeded by: #4867

@losipiuk Please close this PR.

@dain dain closed this Mar 25, 2016
@arhimondr arhimondr deleted the kerberos_hive_v2 branch April 4, 2016 12:43
tanjialiang added a commit to tanjialiang/presto that referenced this pull request Apr 12, 2023
Summary:
* Change include file to be compatible with Velox after following PR
* Advance Velox Version

X-link: facebookincubator/velox#4576

Reviewed By: amitkdutta

Differential Revision: D44875623

Pulled By: tanjialiang

fbshipit-source-id: d9dcab82ac64b0601daf7cb35324ac951d936607
tanjialiang added a commit that referenced this pull request Apr 12, 2023
Summary:
* Change include file to be compatible with Velox after following PR
* Advance Velox Version

X-link: facebookincubator/velox#4576

Reviewed By: amitkdutta

Differential Revision: D44875623

Pulled By: tanjialiang

fbshipit-source-id: d9dcab82ac64b0601daf7cb35324ac951d936607
wanglinsong pushed a commit that referenced this pull request May 17, 2023
Summary:
* Change include file to be compatible with Velox after following PR
* Advance Velox Version

X-link: facebookincubator/velox#4576

Reviewed By: amitkdutta

Differential Revision: D44875623

Pulled By: tanjialiang

fbshipit-source-id: d9dcab82ac64b0601daf7cb35324ac951d936607
jaystarshot pushed a commit to jaystarshot/presto that referenced this pull request Nov 28, 2023
Summary:
* Change include file to be compatible with Velox after following PR
* Advance Velox Version

X-link: facebookincubator/velox#4576

Reviewed By: amitkdutta

Differential Revision: D44875623

Pulled By: tanjialiang

fbshipit-source-id: d9dcab82ac64b0601daf7cb35324ac951d936607

(cherry picked from commit 78104b4)
wypb pushed a commit to wypb/presto that referenced this pull request Dec 22, 2023
Summary:
* Change include file to be compatible with Velox after following PR
* Advance Velox Version

X-link: facebookincubator/velox#4576

Reviewed By: amitkdutta

Differential Revision: D44875623

Pulled By: tanjialiang

fbshipit-source-id: d9dcab82ac64b0601daf7cb35324ac951d936607
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.

6 participants