You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/12/02 12:20:59 UTC
[GitHub] [accumulo] markjens opened a new pull request #2374: Cache the credentials because they are expensive to fetch
markjens opened a new pull request #2374:
URL: https://github.com/apache/accumulo/pull/2374
See the discussion and the thread dumps at https://lists.apache.org/thread/rwt4nomkmhty1dy4tsp620sf49o3qnbx
Without this improvement org.apache.accumulo.test.functional.ConcurrentDeleteTableIT takes 785.503 s on my machine, and I need to use -Dtimeout.factor=3 to pass.
With this improvement the test now passes in 127.771 s, i.e. almost 6 times faster!
<details><summary>thread stack</summary>
"tablet migration-Worker-1" #4380 daemon prio=5 os_prio=0 cpu=68425.44ms elapsed=75.42s tid=0x0000fffeac074800 nid=0x33077e runnable [0x0000fffe8f3fd000]
java.lang.Thread.State: RUNNABLE
at sun.security.provider.SHA5.implCompressCheck(java.base@11.0.11/SHA5.java:232)
at sun.security.provider.SHA5.implCompress(java.base@11.0.11/SHA5.java:221)
at sun.security.provider.DigestBase.engineUpdate(java.base@11.0.11/DigestBase.java:124)
at java.security.MessageDigest$Delegate.engineUpdate(java.base@11.0.11/MessageDigest.java:623)
at java.security.MessageDigest.update(java.base@11.0.11/MessageDigest.java:345)
at org.apache.commons.codec.digest.Sha2Crypt.sha2Crypt(Sha2Crypt.java:421)
at org.apache.commons.codec.digest.Sha2Crypt.sha512Crypt(Sha2Crypt.java:585)
at org.apache.commons.codec.digest.Crypt.crypt(Crypt.java:78)
at org.apache.commons.codec.digest.Crypt.crypt(Crypt.java:167)
at org.apache.accumulo.server.security.SystemCredentials$SystemToken.hashInstanceConfigs(SystemCredentials.java:120)
at org.apache.accumulo.server.security.SystemCredentials$SystemToken.generate(SystemCredentials.java:125)
at org.apache.accumulo.server.security.SystemCredentials.get(SystemCredentials.java:66)
at org.apache.accumulo.server.ServerInfo.getCredentials(ServerInfo.java:179)
at org.apache.accumulo.server.ServerInfo.getPrincipal(ServerInfo.java:148)
at org.apache.accumulo.server.ServerInfo.getProperties(ServerInfo.java:169)
at org.apache.accumulo.core.clientImpl.ClientContext.getProperties(ClientContext.java:236)
at org.apache.accumulo.core.clientImpl.ClientContext.createScanner(ClientContext.java:635)
at org.apache.accumulo.core.metadata.schema.TabletsMetadata$Builder.buildNonRoot(TabletsMetadata.java:177)
at org.apache.accumulo.core.metadata.schema.TabletsMetadata$Builder.build(TabletsMetadata.java:125)
at org.apache.accumulo.core.metadata.schema.AmpleImpl.readTablet(AmpleImpl.java:46)
at org.apache.accumulo.core.metadata.schema.Ample.readTablet(Ample.java:141)
at org.apache.accumulo.tserver.tablet.Tablet.closeConsistencyCheck(Tablet.java:1379)
at org.apache.accumulo.tserver.tablet.Tablet.completeClose(Tablet.java:1331)
- locked <0x00000000f1585830> (a org.apache.accumulo.tserver.tablet.Tablet)
at org.apache.accumulo.tserver.tablet.Tablet.close(Tablet.java:1221)
at org.apache.accumulo.tserver.UnloadTabletHandler.run(UnloadTabletHandler.java:92)
at io.opentelemetry.context.Context.lambda$wrap$1(Context.java:207)
at io.opentelemetry.context.Context$$Lambda$209/0x000000010035c840.run(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@11.0.11/ThreadPoolExecutor.java:1128)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@11.0.11/ThreadPoolExecutor.java:628)
at io.opentelemetry.context.Context.lambda$wrap$1(Context.java:207)
at io.opentelemetry.context.Context$$Lambda$209/0x000000010035c840.run(Unknown Source)
at java.lang.Thread.run(java.base@11.0.11/Thread.java:829)
</details>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2374: Cache the credentials because they are expensive to fetch
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2374:
URL: https://github.com/apache/accumulo/pull/2374#discussion_r766135485
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerInfo.java
##########
@@ -175,10 +179,14 @@ public String getInstanceName() {
return instanceName;
}
- public Credentials getCredentials() {
+ private Credentials loadCredentials() {
return SystemCredentials.get(getInstanceID(), getSiteConfiguration());
}
Review comment:
This `loadCredentials` method should be inline'd into the constructor, and use the instanceId and siteConfig variables available in those constructors, to avoid calling instance methods on a half-constructed object. Since that change is trivial, I can go ahead and do it and merge this in.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #2374: Cache the credentials because they are expensive to fetch
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2374:
URL: https://github.com/apache/accumulo/pull/2374#issuecomment-986220077
Running an a large instance with 16 cores / 32 threads, 128G memory and using SSDs I see some improvement, but not as drastic as reported by others.
There seems to be two modes that occur with some runs being ~90 seconds faster on my instance. In 10 runs on the current main branch, the faster runs occurred twice with an average run time of 162.7 seconds, and eight runs with an average of 252.8 seconds.
Using this PR and 20 runs, the faster runs occurred five times with an average of 83.3 seconds (5.7 seconds faster). The slower runs occurred 15 times with an average of 240.3 seconds (12.5 seconds faster)
# Run time details
<details>
<summary>Click to expand!</summary>
## Run details main branch vs PR with cached credentials
| run | main | cached |
|-----|---------|-----|
| 1 | 256.651 | 242.218 |
| 2 | 164.723 | 155.704 |
| 3 | 250.717 | 241.091 |
| 4 | 250.772 | 239.305 |
| 5 | 160.739 | 245.015 |
| 6 | 253.339 | 155.78 |
| 7 | 253.828 | 238.795 |
| 8 | 254.756 | 241.777 |
| 9 | 250.276 | 236.324 |
| 10 | 251.828 | 160.737 |
| 11 | | 241.228 |
| 12 | | 238.235 |
| 13 | | 160.254 |
| 14 | | 239.816 |
| 15 | | 242.269 |
| 16 | | 152.27 |
| 17 | | 239.339 |
| 18 | | 240.57 |
| 19 | | 239.227 |
| 20 | | 239.218 |
</details>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] markjens commented on pull request #2374: Cache the credentials because they are expensive to fetch
Posted by GitBox <gi...@apache.org>.
markjens commented on pull request #2374:
URL: https://github.com/apache/accumulo/pull/2374#issuecomment-985331437
I've moved the initialization to the constructors!
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2374: Cache the credentials because they are expensive to fetch
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2374:
URL: https://github.com/apache/accumulo/pull/2374#discussion_r761266670
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerInfo.java
##########
@@ -176,7 +177,10 @@ public String getInstanceName() {
}
public Credentials getCredentials() {
- return SystemCredentials.get(getInstanceID(), getSiteConfiguration());
+ if (credentials == null) {
+ credentials = SystemCredentials.get(getInstanceID(), getSiteConfiguration());
Review comment:
This probably needs sync, volatile, or an AtomicReference to be thread safe.
This comment may be moot because of the other comment @ctubbsii made about moving this to the constructor which would make it thread safe.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2374: Cache the credentials because they are expensive to fetch
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2374:
URL: https://github.com/apache/accumulo/pull/2374#issuecomment-985502942
@markjens your changes look good. Can you run "mvn clean verify -DskipTests -Dspotbugs.skip" and push another update? There are some automated QA checks that are failing.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org