Skip to content

KNOX-2658 - Skipping in-memory lookup while fetching expiration/metadata for a token#490

Merged
smolnar82 merged 1 commit intoapache:masterfrom
smolnar82:KNOX-2658
Sep 9, 2021
Merged

KNOX-2658 - Skipping in-memory lookup while fetching expiration/metadata for a token#490
smolnar82 merged 1 commit intoapache:masterfrom
smolnar82:KNOX-2658

Conversation

@smolnar82
Copy link
Copy Markdown
Contributor

@smolnar82 smolnar82 commented Sep 7, 2021

What changes were proposed in this pull request?

As described in the JIRA; in-memory lookup is skipped in JDBC token state service while fetching token expiration or metadata. Instead, we go directly to the DB in this implementation.
Since those queries are simple (there is no table join involved) and they rely on indexed columns the is no significant performance issue.

How was this patch tested?

Updated JUnit test cases and repeated the same steps as described in the JIRA. With my fix, the disabled authentication request fails on node 2 like this:

$ curl -ku Passcode:WXpVNVlUZGlNRGt0WWpFMk5TMDBZamxsTFRobVpEY3ROV0psWW1WbE1EVTRZakF4OjpOVFptWW1VNVlXVXROelppTVMwME5URmhMVGcxWXpRdFl6Z3hNVEUwTmpkak5XUTA= https://localhost:8444/gateway/tokenbased/webhdfs/v1?op=LISTSTATUS
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 401 Token c59a7b09...5bebee058b01 is disabled</title>
</head>
<body><h2>HTTP ERROR 401 Token c59a7b09...5bebee058b01 is disabled</h2>
<table>
<tr><th>URI:</th><td>/gateway/tokenbased/webhdfs/v1</td></tr>
<tr><th>STATUS:</th><td>401</td></tr>
<tr><th>MESSAGE:</th><td>Token c59a7b09...5bebee058b01 is disabled</td></tr>
<tr><th>SERVLET:</th><td>tokenbased-knox-gateway-servlet</td></tr>
</table>

</body>
</html>

@smolnar82 smolnar82 self-assigned this Sep 7, 2021
@smolnar82
Copy link
Copy Markdown
Contributor Author

smolnar82 commented Sep 7, 2021

Cc. @zeroflag

@zeroflag
Copy link
Copy Markdown
Contributor

zeroflag commented Sep 7, 2021

LGTM

moresandeep
moresandeep requested changes Sep 7, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@moresandeep moresandeep left a comment

Choose a reason for hiding this comment

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

Github does not let me click on line numbers that have not changed so adding my comments here. Why do we have some places that use in-memory implementation and some places that do not? it is a bit confusing.

I think instead of removing the token completely can we adjust the TTL just for the metadata in case of JDBC to be short, say 3-5 mins and we can have that as a "margin of error". Or instead of removing the code, use a switch to figure out when HA is enabled so that non-HA JDBC implementations can still benefit from the cache performance. Thoughts?

@smolnar82
Copy link
Copy Markdown
Contributor Author

smolnar82 commented Sep 8, 2021

Thanks, @moresandeep for the detailed comment. Let me try to answer your questions.

Github does not let me click on line numbers that have not changed so adding my comments here. Why do we have some places that use in-memory implementation and some places that do not? it is a bit confusing.

I tried to keep the in-memory lookups where we fetch data that cannot be changed. So that it's gonna be the same on each node all the time (until the token is removed/expired).

I think instead of removing the token completely can we adjust the TTL just for the metadata in case of JDBC to be short, say 3-5 mins and we can have that as a "margin of error". Or instead of removing the code, use a switch to figure out when HA is enabled so that non-HA JDBC implementations can still benefit from the cache performance. Thoughts?

I'd the idea of replacing the current ConcurrentHashMap caches into Caffeine.Cache instances but we would still have an issue with eventual consistency within the configured entry TTL in those caches ("margin of error"). I discussed this approach with @zeroflag and we felt that it'd be pre-mature optimization that can be added later if we hit any performance issues. On the other hand, I'm open to re-evaluate that area if we insist this has to be done now.

@moresandeep
Copy link
Copy Markdown
Contributor

moresandeep commented Sep 8, 2021

I'd the idea of replacing the current ConcurrentHashMap caches into Caffeine.Cache instances but we would still have an issue with eventual consistency within the configured entry TTL in those caches ("margin of error"). I discussed this approach with @zeroflag and we felt that it'd be pre-mature optimization that can be added later if we hit any performance issues. On the other hand, I'm open to re-evaluate that area if we insist this has to be done now.

I see, I am cool with it knowing this has been discussed and will be addressed in the future patches :)
Thanks!

@moresandeep moresandeep self-requested a review September 8, 2021 13:13
@smolnar82
Copy link
Copy Markdown
Contributor Author

smolnar82 commented Sep 9, 2021

I'd the idea of replacing the current ConcurrentHashMap caches into Caffeine.Cache instances but we would still have an issue with eventual consistency within the configured entry TTL in those caches ("margin of error"). I discussed this approach with @zeroflag and we felt that it'd be pre-mature optimization that can be added later if we hit any performance issues. On the other hand, I'm open to re-evaluate that area if we insist this has to be done now.

I see, I am cool with it knowing this has been discussed and will be addressed in the future patches :)
Thanks!

https://issues.apache.org/jira/browse/KNOX-2660

@smolnar82 smolnar82 merged commit 486abb0 into apache:master Sep 9, 2021
@smolnar82 smolnar82 deleted the KNOX-2658 branch September 9, 2021 07:33
stoty pushed a commit to stoty/knox that referenced this pull request May 14, 2024
@smolnar82
CDPD-28979 KNOX-2658 - Skipping in-memory lookup while fetching expir…
9748391 
…ation/metadata for a token (apache#490)

Change-Id: I9836e03efd4d5e37ad7e30ea9806119910b5e7dc
stoty pushed a commit to stoty/knox that referenced this pull request May 14, 2024
@smolnar82
Merge "CDPD-28979 KNOX-2658 - Skipping in-memory lookup while fetchin…
d50c022 
…g expiration/metadata for a token (apache#490)" into cdpd-master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moresandeep moresandeep moresandeep approved these changes

@pzampino pzampino Awaiting requested review from pzampino

Assignees

@smolnar82 smolnar82

Labels

knoxtoken

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@smolnar82 @zeroflag @moresandeep