HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() #1454

adoroszlai · 2020-09-29T14:46:50Z

What changes were proposed in this pull request?

Reduce the number of getCurrentUser() and getTokens() calls performed during some ContainerProtocolCalls operations. This is achieved by getting the tokens once in BlockInputStream and BlockOutputStream initialization, and passing them to getBlock, putBlock, readChunk, writeChunk calls.

https://issues.apache.org/jira/browse/HDDS-4285

How was this patch tested?

Verified that time required for the repro unit test is improved.

Without the patch:

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 112.829 s - in org.apache.hadoop.ozone.client.io.TestKeyOutputStreamUnit

With the patch:

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 30.293 s - in org.apache.hadoop.ozone.client.io.TestKeyOutputStreamUnit

Regular CI:
https://github.com/adoroszlai/hadoop-ozone/runs/1181129921

…erCall()

linyiqun

The fix looks good to me, @adoroszlai .
One minor comment below, : ).

linyiqun · 2020-09-29T15:33:17Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

@@ -84,7 +86,8 @@ private ContainerProtocolCalls() {
   * @throws IOException if there is an I/O error while performing the call
   */
  public static GetBlockResponseProto getBlock(XceiverClientSpi xceiverClient,
-      DatanodeBlockID datanodeBlockID) throws IOException {
+      DatanodeBlockID datanodeBlockID,
+      Collection<Token<? extends TokenIdentifier>> tokens) throws IOException {


Can we update the javadoc comment of this method as we new added param tokens here?
The same suggestion for methods we changed below:

putBlockAsync

readChunk

writeChunkAsync

Thanks @linyiqun for the suggestion. Would

@param tokens list of tokens the current user has, possibly including a token for this block

be OK, or do you have a better description for this param?

@adoroszlai , above description looks good to me. Thanks.

We can directly encode the pass in token for the specific block id without additional token selection.

ChenSammi · 2020-09-30T04:06:09Z

@adoroszlai thanks for identify this performance issue. I'm not quite familar with tokens, since it's a user token, suppose it has a much longer liveness than input and output stream object instance, right?

adoroszlai · 2020-09-30T09:12:17Z

thanks for identify this performance issue

Credit goes to @elek who reported the problem, see HDDS-4285.

I'm not quite familar with tokens

Neither am I, but we have some doc.

since it's a user token, suppose it has a much longer liveness than input and output stream object instance, right?

It is a block token, specific to the block being written/read. I think it's short-lived.

https://github.com/apache/hadoop-ozone/blob/d1ac423b3fe35c5d363b348108512440f08ee5f1/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/OzoneBlockTokenIdentifier.java#L41-L45

linyiqun · 2020-09-30T13:13:01Z

since it's a user token, suppose it has a much longer liveness than input and output stream object instance, right?

A good point that @ChenSammi mentioned, there is a setting to decide the expired time of block token and it's a long time as 1 day. This should be an enough time for client to read/write block data.

  <property>
    <name>hdds.block.token.expiry.time</name>
    <value>1d</value>
    <tag>OZONE, HDDS, SECURITY, TOKEN</tag>
    <description>
      Default value for expiry time of block token. This
      setting supports multiple time unit suffixes as described in
      dfs.heartbeat.interval. If no suffix is specified, then milliseconds is
      assumed.
    </description>
  </property>

No need to renew or revoke a block access token. when a cached block access token expires, the client simply get a new one. Block access token should be cached only in memory and never write to disk.

From this comment of OzoneBlockTokenIdentifier, current change also looks safe.

In additional, I'm +1 for current change. Thanks @adoroszlai for addressing the comment.

xiaoyuyao · 2020-09-30T16:17:01Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

@@ -193,10 +196,11 @@ public synchronized void initialize() throws IOException {
      if (token != null) {
        UserGroupInformation.getCurrentUser().addToken(token);


Should we just pass down the token from the caller directly and bypass the UGI addToken/getTokens completely?

xiaoyuyao

Thanks @adoroszlai for working on this. The change LGTM overall. Just have few comments added inline.

xiaoyuyao · 2020-09-30T16:24:28Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

@@ -84,7 +86,8 @@ private ContainerProtocolCalls() {
   * @throws IOException if there is an I/O error while performing the call
   */
  public static GetBlockResponseProto getBlock(XceiverClientSpi xceiverClient,
-      DatanodeBlockID datanodeBlockID) throws IOException {
+      DatanodeBlockID datanodeBlockID,
+      Collection<Token<? extends TokenIdentifier>> tokens) throws IOException {


We can directly encode the pass in token for the specific block id without additional token selection.

adoroszlai · 2020-09-30T17:04:35Z

Should we just pass down the token from the caller directly and bypass the UGI addToken/getTokens completely?
We can directly encode the pass in token for the specific block id without additional token selection.

Thanks @xiaoyuyao for the comments. I was also wondering about this, but I didn't have enough background info to decide. If you feel comfortable with bypassing the UGI for the block token, I'm fine with making this simplification.

adoroszlai · 2020-10-08T16:11:06Z

Thanks @xiaoyuyao for the suggestion to directly pass down the block token. I have updated the patch accordingly.

ChenSammi · 2020-10-09T03:25:34Z

@adoroszlai ， thanks for the explanation. +1.

xiaoyuyao · 2020-10-09T15:28:31Z

Thanks @adoroszlai for the update. LGTM, +1.

adoroszlai · 2020-10-09T21:26:06Z

Thanks @xiaoyuyao for reviewing and committing it, @ChenSammi and @linyiqun for the review, and @elek for finding the issue.

* master: (23 commits) HDDS-4122. Implement OM Delete Expired Open Key Request and Response (apache#1435) HDDS-4336. ContainerInfo does not persist BCSID (sequenceId) leading to failed replica reports (apache#1488) Remove extra serialization from getBlockID (apache#1470) HDDS-4262. Use ClientID and CallID from Rpc Client to detect retry requests (apache#1436) HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() (apache#1454) HDDS-4312. findbugs check succeeds despite compile error (apache#1476) HDDS-4311. Type-safe config design doc points to OM HA (apache#1477) HDDS-3814. Drop a column family through debug cli tool (apache#1083) HDDS-3728. Bucket space: check quotaUsageInBytes when write key and allocate block. (apache#1458) HDDS-4316. Upgrade to angular 1.8.0 due to CVE-2020-7676 (apache#1481) HDDS-4325. Incompatible return codes from Ozone getconf -confKey (apache#1485). Contributed by Doroszlai, Attila. HDDS-4309. Fix inconsistency in recon config keys starting with recon and not ozone (apache#1478) HDDS-4310: Ozone getconf broke the compatibility (apache#1475) HDDS-4298. Use an interface in Ozone client instead of XceiverClientManager (apache#1460) HDDS-4280. Document notable configurations for Recon. (apache#1448) HDDS-4156. add hierarchical layout to Chinese doc (apache#1368) HDDS-4242. Copy PrefixInfo proto to new project hadoop-ozone/interface-storage (apache#1444) HDDS-4264. Uniform naming conventions of Ozone Shell Options. (apache#1447) HDDS-4271. Avoid logging chunk content in Ozone Insight (apache#1466) HDDS-4299. Display Ratis version with ozone version (apache#1464) ...

adoroszlai added 3 commits September 28, 2020 17:56

HDDS-4285. Read is slow due to the frequent usage of UGI.getCurrentUs…

86dbee8

…erCall()

Fix read path

32387fc

trigger new CI check

980842e

adoroszlai self-assigned this Sep 29, 2020

adoroszlai changed the title ~~HDDS-4285. Read is slow due to the frequent usage of UGI.getCurrentUser Call()~~ HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() Sep 29, 2020

linyiqun reviewed Sep 29, 2020

View reviewed changes

Add javadoc for new param

3fd7d07

xiaoyuyao reviewed Sep 30, 2020

View reviewed changes

adoroszlai added enhancement New feature or request performance labels Oct 7, 2020

adoroszlai added 2 commits October 8, 2020 10:58

HDDS-4285. Directly pass block token

e05eb00

Merge remote-tracking branch 'origin/master' into HDDS-4285

5617fac

xiaoyuyao merged commit 35cc6b0 into apache:master Oct 9, 2020

adoroszlai deleted the HDDS-4285 branch October 9, 2020 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() #1454

HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() #1454

adoroszlai commented Sep 29, 2020

linyiqun left a comment

linyiqun Sep 29, 2020

adoroszlai Sep 29, 2020

linyiqun Sep 30, 2020

xiaoyuyao Sep 30, 2020

ChenSammi commented Sep 30, 2020

adoroszlai commented Sep 30, 2020

linyiqun commented Sep 30, 2020 •

edited

Loading

xiaoyuyao Sep 30, 2020

xiaoyuyao left a comment

xiaoyuyao Sep 30, 2020

adoroszlai commented Sep 30, 2020

adoroszlai commented Oct 8, 2020

ChenSammi commented Oct 9, 2020

xiaoyuyao commented Oct 9, 2020

adoroszlai commented Oct 9, 2020

		@@ -193,10 +196,11 @@ public synchronized void initialize() throws IOException {
		if (token != null) {
		UserGroupInformation.getCurrentUser().addToken(token);

HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() #1454

HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() #1454

Conversation

adoroszlai commented Sep 29, 2020

What changes were proposed in this pull request?

How was this patch tested?

linyiqun left a comment

Choose a reason for hiding this comment

linyiqun Sep 29, 2020

Choose a reason for hiding this comment

adoroszlai Sep 29, 2020

Choose a reason for hiding this comment

linyiqun Sep 30, 2020

Choose a reason for hiding this comment

xiaoyuyao Sep 30, 2020

Choose a reason for hiding this comment

ChenSammi commented Sep 30, 2020

adoroszlai commented Sep 30, 2020

linyiqun commented Sep 30, 2020 • edited Loading

xiaoyuyao Sep 30, 2020

Choose a reason for hiding this comment

xiaoyuyao left a comment

Choose a reason for hiding this comment

xiaoyuyao Sep 30, 2020

Choose a reason for hiding this comment

adoroszlai commented Sep 30, 2020

adoroszlai commented Oct 8, 2020

ChenSammi commented Oct 9, 2020

xiaoyuyao commented Oct 9, 2020

adoroszlai commented Oct 9, 2020

linyiqun commented Sep 30, 2020 •

edited

Loading