You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "hchaverri (via GitHub)" <gi...@apache.org> on 2023/07/28 00:43:42 UTC
[GitHub] [hadoop] hchaverri opened a new pull request, #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
hchaverri opened a new pull request, #5897:
URL: https://github.com/apache/hadoop/pull/5897
…che so tokens are updated frequently
<!--
Thanks for sending a pull request!
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute
2. Make sure your PR title starts with JIRA issue id, e.g., 'HADOOP-17799. Your PR title ...'.
-->
### Description of PR
The SQLDelegationTokenSecretManager is used by RBF to store a higher volume of tokens than supported by Zookeeper. Currently, the default in-memory Map is used to store tokens on each router and its contents are not refreshed periodically. These changes will allow routers to update the status of tokens in memory after a short period of time, such that renewals or cancellations handled by any router are reflected on all of them.
### How was this patch tested?
Added unit tests for renewal and cancellations, validating that changes are propagated to other SecretManagers.
### For code changes:
- [Y] Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
- [Y] Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
- [Y] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [Y] If applicable, have you updated the `LICENSE`, `LICENSE-binary`, `NOTICE-binary` files?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1654950638
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 14m 13s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 20m 34s | | trunk passed |
| +1 :green_heart: | compile | 10m 32s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | compile | 9m 21s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | checkstyle | 2m 21s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 55s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 42s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 20s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 44s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 24s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 26s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 1m 1s | | the patch passed |
| +1 :green_heart: | compile | 9m 50s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javac | 9m 50s | | the patch passed |
| +1 :green_heart: | compile | 9m 35s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | javac | 9m 35s | | the patch passed |
| -1 :x: | blanks | 0m 0s | [/blanks-eol.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/1/artifact/out/blanks-eol.txt) | The patch has 2 line(s) that end in blanks. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| -0 :warning: | checkstyle | 2m 20s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/1/artifact/out/results-checkstyle-root.txt) | root: The patch generated 1 new + 23 unchanged - 1 fixed = 24 total (was 24) |
| +1 :green_heart: | mvnsite | 1m 53s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 35s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 3m 2s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 47s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 36s | | hadoop-common in the patch passed. |
| +1 :green_heart: | unit | 19m 3s | | hadoop-hdfs-rbf in the patch passed. |
| +1 :green_heart: | asflicense | 0m 52s | | The patch does not generate ASF License warnings. |
| | | 180m 45s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5897 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux f224e799b764 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 1cbd92273398e2683d2622c96aae71afcf34ba9f |
| Default Java | Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/1/testReport/ |
| Max. process+thread count | 2892 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-rbf U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/1/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1278018944
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -771,10 +771,14 @@ protected void logExpireTokens(
for (TokenIdent ident : expiredTokens) {
logExpireToken(ident);
LOG.info("Removing expired token " + formatTokenId(ident));
Review Comment:
I'm trying to follow the behavior of the other SecretManagers that extend this class, where they log this line and later log an error if anything failed. Some examples on `JHSDelegationTokenSecretManager`, `RMDelegationTokenSecretManager` and `ZKDelegationTokenSecretManager`
I will add a bit more logging on the SQLDTSM to call out if/why the token was not removed. Do you think this will be enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1282301391
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
Review Comment:
What number are you referring to?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1280112864
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -153,36 +161,60 @@ protected void removeStoredToken(TokenIdent ident) throws IOException {
}
}
+ @Override
+ protected void removeExpiredStoredToken(TokenIdent ident) {
+ try {
+ // Ensure that the token has not been renewed in SQL by
+ // another secret manager
+ DelegationTokenInformation tokenInfo = getTokenInfoFromSQL(ident);
+ if (tokenInfo.getRenewDate() >= Time.now()) {
+ LOG.info("Token was renewed by a different router and has not been deleted: " + ident);
Review Comment:
LOG.info("Token was renewed by a different router and has not been deleted: {}", ident);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1665507004
@hchaverri We need to fix the checkstyle issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1278114605
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -46,6 +50,9 @@ public abstract class SQLDelegationTokenSecretManager<TokenIdent
private static final String SQL_DTSM_TOKEN_SEQNUM_BATCH_SIZE = SQL_DTSM_CONF_PREFIX
+ "token.seqnum.batch.size";
public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;
+ public static final String SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_MS = SQL_DTSM_CONF_PREFIX
+ + "token.loading.cache.expiration.ms";
+ public static final int SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_DEFAULT_MS = 10000;
Review Comment:
In the tests that I've run, with a lower renew interval, I've seen renewals come 30 seconds before the expiration time. In our live service I see renewals happening 2 hours before expiration. I've yet to find how to configure it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1278063599
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -771,10 +771,14 @@ protected void logExpireTokens(
for (TokenIdent ident : expiredTokens) {
logExpireToken(ident);
LOG.info("Removing expired token " + formatTokenId(ident));
Review Comment:
Yes, I think it will be enough to log the reason the token was not removed. So we'll be able to reconstruct from the logs what happened.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1280115270
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
Review Comment:
Do we need to increase the limit of a number?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1282301620
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -46,6 +50,9 @@ public abstract class SQLDelegationTokenSecretManager<TokenIdent
private static final String SQL_DTSM_TOKEN_SEQNUM_BATCH_SIZE = SQL_DTSM_CONF_PREFIX
+ "token.seqnum.batch.size";
public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;
+ public static final String SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_MS = SQL_DTSM_CONF_PREFIX
+ + "token.loading.cache.expiration.ms";
Review Comment:
I agree, I've updated this
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -46,6 +50,9 @@ public abstract class SQLDelegationTokenSecretManager<TokenIdent
private static final String SQL_DTSM_TOKEN_SEQNUM_BATCH_SIZE = SQL_DTSM_CONF_PREFIX
+ "token.seqnum.batch.size";
public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;
+ public static final String SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_MS = SQL_DTSM_CONF_PREFIX
+ + "token.loading.cache.expiration.ms";
+ public static final int SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_DEFAULT_MS = 10000;
Review Comment:
Updated, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1664602549
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 8m 30s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 14m 14s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 20m 7s | | trunk passed |
| +1 :green_heart: | compile | 10m 35s | | trunk passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | compile | 9m 28s | | trunk passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | checkstyle | 2m 21s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 55s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 42s | | trunk passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | javadoc | 1m 21s | | trunk passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | spotbugs | 2m 45s | | trunk passed |
| +1 :green_heart: | shadedclient | 22m 16s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 24s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 1m 1s | | the patch passed |
| +1 :green_heart: | compile | 9m 41s | | the patch passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | javac | 9m 41s | | the patch passed |
| +1 :green_heart: | compile | 9m 30s | | the patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | javac | 9m 30s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 2m 20s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/5/artifact/out/results-checkstyle-root.txt) | root: The patch generated 2 new + 23 unchanged - 1 fixed = 25 total (was 24) |
| +1 :green_heart: | mvnsite | 1m 57s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 36s | | the patch passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | javadoc | 1m 20s | | the patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | spotbugs | 2m 56s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 51s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 18s | | hadoop-common in the patch passed. |
| +1 :green_heart: | unit | 19m 4s | | hadoop-hdfs-rbf in the patch passed. |
| +1 :green_heart: | asflicense | 0m 51s | | The patch does not generate ASF License warnings. |
| | | 190m 11s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5897 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux e444c8c2ca83 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 3a6a70ec8308640199ad73b222a7cee2b95ee50f |
| Default Java | Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/5/testReport/ |
| Max. process+thread count | 2671 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-rbf U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/5/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1282657547
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
Review Comment:
Should we consider the limit on the number of caches, because if there is no limit on the number of caches, will it cause memory shortage?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1655301411
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 30s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 13m 23s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 20m 0s | | trunk passed |
| +1 :green_heart: | compile | 10m 25s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | compile | 9m 34s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | checkstyle | 2m 23s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 59s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 42s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 43s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 36s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 26s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 1m 2s | | the patch passed |
| +1 :green_heart: | compile | 9m 43s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javac | 9m 43s | | the patch passed |
| +1 :green_heart: | compile | 10m 32s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | javac | 10m 32s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 2m 45s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/2/artifact/out/results-checkstyle-root.txt) | root: The patch generated 1 new + 23 unchanged - 1 fixed = 24 total (was 24) |
| +1 :green_heart: | mvnsite | 1m 54s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 37s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 56s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 40s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 15s | | hadoop-common in the patch passed. |
| +1 :green_heart: | unit | 19m 4s | | hadoop-hdfs-rbf in the patch passed. |
| +1 :green_heart: | asflicense | 0m 52s | | The patch does not generate ASF License warnings. |
| | | 180m 28s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5897 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux f3f0bc5a4564 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 678366177d5fd16382986deb9fe9c8f66e60f528 |
| Default Java | Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/2/testReport/ |
| Max. process+thread count | 2594 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-rbf U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/2/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1279656588
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
+ .build(new CacheLoader<K, V>() {
+ @Override
+ public V load(K k) throws Exception {
+ return singleEntryFunction.apply(k);
+ }
+ });
Review Comment:
Thanks. Would `Cache.asMap()` be sufficient for these use cases.
So doing
```
currentToken = CacheBuilder.newBuilder()
...
.build()
.asMap()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1277987367
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -46,6 +50,9 @@ public abstract class SQLDelegationTokenSecretManager<TokenIdent
private static final String SQL_DTSM_TOKEN_SEQNUM_BATCH_SIZE = SQL_DTSM_CONF_PREFIX
+ "token.seqnum.batch.size";
public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;
+ public static final String SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_MS = SQL_DTSM_CONF_PREFIX
+ + "token.loading.cache.expiration.ms";
+ public static final int SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_DEFAULT_MS = 10000;
Review Comment:
Can you comment on how `SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_DEFAULT_MS` interacts with `DelegationTokenManager.REMOVAL_SCAN_INTERVAL_DEFAULT`.
When the only concern is not removing renewed token, then then loading cache expiry doesn't have to be as aggressive. 10 seconds, vs. expired tokens being checked every hour.
For token cancellations though, I can see the need for a more aggressive expiry though 10 seconds may lead to too much load on the databases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1663028496
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 14m 25s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 19m 53s | | trunk passed |
| +1 :green_heart: | compile | 10m 35s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | compile | 9m 36s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | checkstyle | 2m 23s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 57s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 41s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 20s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 49s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 41s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 25s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 1m 1s | | the patch passed |
| +1 :green_heart: | compile | 9m 45s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javac | 9m 45s | | the patch passed |
| +1 :green_heart: | compile | 9m 45s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | javac | 9m 45s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 2m 18s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/4/artifact/out/results-checkstyle-root.txt) | root: The patch generated 1 new + 23 unchanged - 1 fixed = 24 total (was 24) |
| +1 :green_heart: | mvnsite | 1m 54s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 36s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 57s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 53s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 20s | | hadoop-common in the patch passed. |
| +1 :green_heart: | unit | 19m 7s | | hadoop-hdfs-rbf in the patch passed. |
| +1 :green_heart: | asflicense | 0m 51s | | The patch does not generate ASF License warnings. |
| | | 181m 39s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5897 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 88ff955e0b78 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / ada9128c26651625ade15ec3aaf0293a09952c8d |
| Default Java | Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/4/testReport/ |
| Max. process+thread count | 2585 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-rbf U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/4/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1668705535
> I've fixed the one introduced with my changes on TestSQLDelegationTokenSecretManagerImpl. The one in AbstractDelegationTokenSecretManager is pre-existing and has 37 references that would need to be updated. Do you recommend fixing them anyway?
@hchaverri Thank you very much for your contribution! For the existing problem of history, we don't need to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1668713106
@hchaverri merged into trunk. @goiri @simbadzina Thanks for helping to review the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 merged pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 merged PR #5897:
URL: https://github.com/apache/hadoop/pull/5897
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1277966235
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -771,10 +771,14 @@ protected void logExpireTokens(
for (TokenIdent ident : expiredTokens) {
logExpireToken(ident);
LOG.info("Removing expired token " + formatTokenId(ident));
Review Comment:
`Removing expired token` will be logged even when the token is then not removed.
`removeExpiredStoredToken` could return a boolean indicating whether an token was actually removed. We can use this value to determine whether to print the log line or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1278071362
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -46,6 +50,9 @@ public abstract class SQLDelegationTokenSecretManager<TokenIdent
private static final String SQL_DTSM_TOKEN_SEQNUM_BATCH_SIZE = SQL_DTSM_CONF_PREFIX
+ "token.seqnum.batch.size";
public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;
+ public static final String SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_MS = SQL_DTSM_CONF_PREFIX
+ + "token.loading.cache.expiration.ms";
+ public static final int SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_DEFAULT_MS = 10000;
Review Comment:
Makes sense. I hadn't thought about stale tokens.
Do you know how early renewal is done? If it is a say half-way into the token's 24 hour lifetime, then there would be more time to refresh the cache.
Anyhow, since this is a configurable value operators can explore what value is appropriate for them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1279644268
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
+ .build(new CacheLoader<K, V>() {
+ @Override
+ public V load(K k) throws Exception {
+ return singleEntryFunction.apply(k);
+ }
+ });
Review Comment:
We mainly need this to implement the Map functions so we don't have to update multiple SecretManger classes. Those classes currently expect to interact with the currentTokens Map and LoadingCaches don't provide all those methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1656476826
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 16m 11s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 20m 12s | | trunk passed |
| +1 :green_heart: | compile | 10m 44s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | compile | 9m 31s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | checkstyle | 2m 24s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 58s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 40s | | trunk passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | trunk passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 43s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 31s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 25s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 1m 2s | | the patch passed |
| +1 :green_heart: | compile | 9m 46s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javac | 9m 46s | | the patch passed |
| +1 :green_heart: | compile | 9m 37s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | javac | 9m 37s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 2m 16s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/3/artifact/out/results-checkstyle-root.txt) | root: The patch generated 1 new + 23 unchanged - 1 fixed = 24 total (was 24) |
| +1 :green_heart: | mvnsite | 1m 55s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 37s | | the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | the patch passed with JDK Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 2m 57s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 52s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 17s | | hadoop-common in the patch passed. |
| +1 :green_heart: | unit | 19m 4s | | hadoop-hdfs-rbf in the patch passed. |
| +1 :green_heart: | asflicense | 0m 51s | | The patch does not generate ASF License warnings. |
| | | 182m 45s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5897 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux e75ee3e0f790 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 700af1a3139422077576bab83f5ab6ad5e640b82 |
| Default Java | Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/3/testReport/ |
| Max. process+thread count | 2565 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-rbf U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/3/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1279691057
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
+ .build(new CacheLoader<K, V>() {
+ @Override
+ public V load(K k) throws Exception {
+ return singleEntryFunction.apply(k);
+ }
+ });
Review Comment:
The `get()` needs to be done on the `LoadingCache` for the `CacheLoader` to trigger. Calling it on the result of `asMap()` will just return null if the item is not in it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1659575245
LGTM +1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1666552826
> @hchaverri We need to fix the checkstyle issue.
I've fixed the one introduced with my changes on TestSQLDelegationTokenSecretManagerImpl. The one in AbstractDelegationTokenSecretManager is pre-existing and has 37 references that would need to be updated. Do you recommend fixing them anyway?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hadoop-yetus (via GitHub)" <gi...@apache.org>.
hadoop-yetus commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1666591415
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +0 :ok: | mvndep | 17m 54s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 20m 18s | | trunk passed |
| +1 :green_heart: | compile | 10m 22s | | trunk passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | compile | 9m 34s | | trunk passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | checkstyle | 2m 21s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 56s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 41s | | trunk passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | javadoc | 1m 21s | | trunk passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | spotbugs | 2m 50s | | trunk passed |
| +1 :green_heart: | shadedclient | 21m 34s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 26s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 1m 0s | | the patch passed |
| +1 :green_heart: | compile | 9m 46s | | the patch passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | javac | 9m 46s | | the patch passed |
| +1 :green_heart: | compile | 9m 29s | | the patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | javac | 9m 29s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 2m 18s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/6/artifact/out/results-checkstyle-root.txt) | root: The patch generated 1 new + 23 unchanged - 1 fixed = 24 total (was 24) |
| +1 :green_heart: | mvnsite | 1m 55s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 37s | | the patch passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 |
| +1 :green_heart: | javadoc | 1m 22s | | the patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| +1 :green_heart: | spotbugs | 3m 1s | | the patch passed |
| +1 :green_heart: | shadedclient | 21m 45s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 16m 19s | | hadoop-common in the patch passed. |
| +1 :green_heart: | unit | 19m 3s | | hadoop-hdfs-rbf in the patch passed. |
| +1 :green_heart: | asflicense | 0m 51s | | The patch does not generate ASF License warnings. |
| | | 183m 51s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/6/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/5897 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 64bf3973a337 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 06adf5158e948a3b6b21dbf99f24e263cb8035d9 |
| Default Java | Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_382-8u382-ga-1~20.04.1-b05 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/6/testReport/ |
| Max. process+thread count | 2580 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-rbf U: . |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5897/6/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1283498038
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
Review Comment:
Yeah I see the risk if users increase the TTL for the cache. I've set a default of 100k which would add up to around 50 MBs with the default TTL of 10 seconds. Thanks for catching this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1279637935
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
+ .build(new CacheLoader<K, V>() {
+ @Override
+ public V load(K k) throws Exception {
+ return singleEntryFunction.apply(k);
+ }
+ });
Review Comment:
Do we need this as a separate class? Is there any other functionality we need other than what is in the constructor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1282301050
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -153,36 +161,60 @@ protected void removeStoredToken(TokenIdent ident) throws IOException {
}
}
+ @Override
+ protected void removeExpiredStoredToken(TokenIdent ident) {
+ try {
+ // Ensure that the token has not been renewed in SQL by
+ // another secret manager
+ DelegationTokenInformation tokenInfo = getTokenInfoFromSQL(ident);
+ if (tokenInfo.getRenewDate() >= Time.now()) {
+ LOG.info("Token was renewed by a different router and has not been deleted: " + ident);
Review Comment:
Thanks. I've updated the logging format
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] simbadzina commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCache so tokens are updated frequently.
Posted by "simbadzina (via GitHub)" <gi...@apache.org>.
simbadzina commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1279712302
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/DelegationTokenLoadingCache.java:
##########
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.security.token.delegation;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheBuilder;
+import org.apache.hadoop.thirdparty.com.google.common.cache.CacheLoader;
+import org.apache.hadoop.thirdparty.com.google.common.cache.LoadingCache;
+
+
+/**
+ * Cache for delegation tokens that can handle high volume of tokens. A
+ * loading cache will prevent all active tokens from being in memory at the
+ * same time. It will also trigger more requests from the persistent token storage.
+ */
+public class DelegationTokenLoadingCache<K, V> implements Map<K, V> {
+ private LoadingCache<K, V> internalLoadingCache;
+
+ public DelegationTokenLoadingCache(long cacheExpirationMs, Function<K, V> singleEntryFunction) {
+ this.internalLoadingCache = CacheBuilder.newBuilder()
+ .expireAfterWrite(cacheExpirationMs, TimeUnit.MILLISECONDS)
+ .build(new CacheLoader<K, V>() {
+ @Override
+ public V load(K k) throws Exception {
+ return singleEntryFunction.apply(k);
+ }
+ });
Review Comment:
Got it, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hchaverri commented on a diff in pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "hchaverri (via GitHub)" <gi...@apache.org>.
hchaverri commented on code in PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#discussion_r1278035871
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -46,6 +50,9 @@ public abstract class SQLDelegationTokenSecretManager<TokenIdent
private static final String SQL_DTSM_TOKEN_SEQNUM_BATCH_SIZE = SQL_DTSM_CONF_PREFIX
+ "token.seqnum.batch.size";
public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;
+ public static final String SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_MS = SQL_DTSM_CONF_PREFIX
+ + "token.loading.cache.expiration.ms";
+ public static final int SQL_DTSM_TOKEN_LOADING_CACHE_EXPIRATION_DEFAULT_MS = 10000;
Review Comment:
The concern is not only with deleting tokens from SQL that have been renewed, but also with tokens being stale in the cache. If a router has a stale token that has been renewed somewhere else, the router will throw an authentication error since the token in memory is expired. We should keep this value low enough so the renewal is propagated to all routers quickly enough. Same reason for cancellations. I can see us tweaking this value to balance the impact on SQL.
The removal scan won't actually help much with token cleanup as we only expect 10 seconds of tokens to be candidates for cleanup. We need a separate mechanism to query SQL for all expired tokens and delete them, but I think we should track that separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] slfan1989 commented on pull request #5897: HDFS-17128. Updating SQLDelegationTokenSecretManager to use LoadingCa…
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #5897:
URL: https://github.com/apache/hadoop/pull/5897#issuecomment-1655049316
Good Catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org