You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by "shekhars-li (via GitHub)" <gi...@apache.org> on 2023/09/25 19:44:35 UTC
[GitHub] [samza] shekhars-li opened a new pull request, #1687: Added cache for file blobs that are already TTL removed
shekhars-li opened a new pull request, #1687:
URL: https://github.com/apache/samza/pull/1687
Problem:
After every commit, we perform a cleanup phase where, among other things, we remove TTL of all the blobs associated with a SnapshotIndex. However, not all files inside a SnapshotIndex changes at every commit. More often, the files are almost identical to previous commit and only a few files are added/removed/updated. This leads to removeTTL for the same blob id repeatedly. While this is an idempotent operation, this is unnecessary. From our logs, sometimes we call removeTTL calls for the same blob 5-10 times.
Solution:
Maintain a simple LRU cache and store the blob id that was TTL removed and check that before making the remote call.
Test:
Added a unit test to validate that the repeated calls for ttl remove are avoided.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shekhars-li commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336420008
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -21,6 +21,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
Review Comment:
We want 2 things:
1. LRU cache of some form - some files tend to stick around for a long time - typically files on higher level in level db. So we want them to be kept around for far longer than files that are merged into the next level or were just created.
2. Fixed size of the cache - we don't want to balloon the cache size to be too big as we create lots of files over time (most of which are deleted in the next commit).
I considered using set but I think Guava cache (which we already are using elsewhere in this module) seemed like a a cleaner way to do 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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shekhars-li commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336424013
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -90,6 +92,9 @@ public class BlobStoreUtil {
private final SnapshotIndexSerde snapshotIndexSerde;
private final RetryPolicyConfig retryPolicyConfig;
+ // LRU cache for blob ids that were TTL removed
+ private final Cache<String, Object> ttlRemovedBlobIdsCache = CacheBuilder.newBuilder().maximumSize(1000).build();
Review Comment:
Based on the metrics I checked for our jobs (some large jobs, didn't have a way to check avg), it's around 100 blobs per minute per container. So it should be able to hold about 10 mins worth of blob ids without any dedup.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] dxichen commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "dxichen (via GitHub)" <gi...@apache.org>.
dxichen commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336400804
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -670,12 +676,18 @@ private CompletableFuture<Void> removeTTL(DirIndex dirIndex, Metadata metadata)
metadata.getJobName(), metadata.getJobId(), metadata.getTaskName(), metadata.getStoreName());
List<FileBlob> fileBlobs = file.getBlobs();
for (FileBlob fileBlob : fileBlobs) {
- String opname = "removeTTL for fileBlob: " + file.getFileName() + " with blobId: {}" + fileBlob.getBlobId();
- Supplier<CompletionStage<Void>> ttlRemovalAction = () ->
- blobStoreManager.removeTTL(fileBlob.getBlobId(), requestMetadata).toCompletableFuture();
- CompletableFuture<Void> ttlRemovalFuture =
- FutureUtil.executeAsyncWithRetries(opname, ttlRemovalAction, isCauseNonRetriable(), executor, retryPolicyConfig);
- updateTTLsFuture.add(ttlRemovalFuture);
+ if (ttlRemovedBlobIdsCache.getIfPresent(fileBlob.getBlobId()) == null) {
+ String opname = "removeTTL for fileBlob: " + file.getFileName() + " with blobId: {}" + fileBlob.getBlobId();
+ Supplier<CompletionStage<Void>> ttlRemovalAction = () ->
+ blobStoreManager.removeTTL(fileBlob.getBlobId(), requestMetadata).toCompletableFuture()
+ .thenRun(() -> ttlRemovedBlobIdsCache.put(fileBlob.getBlobId(), new Object()));
Review Comment:
Lets also store the time this was removed for debugging purposes and add it in the log below
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -90,6 +92,9 @@ public class BlobStoreUtil {
private final SnapshotIndexSerde snapshotIndexSerde;
private final RetryPolicyConfig retryPolicyConfig;
+ // LRU cache for blob ids that were TTL removed
+ private final Cache<String, Object> ttlRemovedBlobIdsCache = CacheBuilder.newBuilder().maximumSize(1000).build();
Review Comment:
Could you verify how many blobs are usually stored per store? we could use that to inform on this constant
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -21,6 +21,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
Review Comment:
Any particular reason we are using an LRU cache instead of an in mem set here? we are not using anything cache specific ie invalidation
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shekhars-li commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336424013
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -90,6 +92,9 @@ public class BlobStoreUtil {
private final SnapshotIndexSerde snapshotIndexSerde;
private final RetryPolicyConfig retryPolicyConfig;
+ // LRU cache for blob ids that were TTL removed
+ private final Cache<String, Object> ttlRemovedBlobIdsCache = CacheBuilder.newBuilder().maximumSize(1000).build();
Review Comment:
Based on the metrics I checked for our jobs (some large jobs, didn't have a way to check avg), it's around 100 blobs per minute for all the stores for a job. So it should be able to hold about 10 mins worth of blob ids without any dedup.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shekhars-li commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336420707
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -670,12 +676,18 @@ private CompletableFuture<Void> removeTTL(DirIndex dirIndex, Metadata metadata)
metadata.getJobName(), metadata.getJobId(), metadata.getTaskName(), metadata.getStoreName());
List<FileBlob> fileBlobs = file.getBlobs();
for (FileBlob fileBlob : fileBlobs) {
- String opname = "removeTTL for fileBlob: " + file.getFileName() + " with blobId: {}" + fileBlob.getBlobId();
- Supplier<CompletionStage<Void>> ttlRemovalAction = () ->
- blobStoreManager.removeTTL(fileBlob.getBlobId(), requestMetadata).toCompletableFuture();
- CompletableFuture<Void> ttlRemovalFuture =
- FutureUtil.executeAsyncWithRetries(opname, ttlRemovalAction, isCauseNonRetriable(), executor, retryPolicyConfig);
- updateTTLsFuture.add(ttlRemovalFuture);
+ if (ttlRemovedBlobIdsCache.getIfPresent(fileBlob.getBlobId()) == null) {
+ String opname = "removeTTL for fileBlob: " + file.getFileName() + " with blobId: {}" + fileBlob.getBlobId();
+ Supplier<CompletionStage<Void>> ttlRemovalAction = () ->
+ blobStoreManager.removeTTL(fileBlob.getBlobId(), requestMetadata).toCompletableFuture()
+ .thenRun(() -> ttlRemovedBlobIdsCache.put(fileBlob.getBlobId(), new Object()));
Review Comment:
ACK. Will add that to the cache and to debug lines. 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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shekhars-li commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336420008
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -21,6 +21,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
Review Comment:
We want 2 things:
1. LRU cache of some form - some files tend to stick around for a long time - typically files on higher level in level db. So we want them to be kept around for far longer than files that are merged into the next level or were just created. This cache is LRU by default and should ensure those blob ids (keys) stick around for as long as it's needed.
2. Fixed size of the cache - we don't want to balloon the cache size to be too big as we create lots of files over time (most of which are deleted in the next commit).
I considered using set but I think Guava cache (which we already are using elsewhere in this module) seemed like a a cleaner way to do 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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added cache for file blobs that are already TTL removed [samza]
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li closed pull request #1687: Added cache for file blobs that are already TTL removed
URL: https://github.com/apache/samza/pull/1687
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shekhars-li commented on a diff in pull request #1687: Added cache for file blobs that are already TTL removed
Posted by "shekhars-li (via GitHub)" <gi...@apache.org>.
shekhars-li commented on code in PR #1687:
URL: https://github.com/apache/samza/pull/1687#discussion_r1336420707
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/util/BlobStoreUtil.java:
##########
@@ -670,12 +676,18 @@ private CompletableFuture<Void> removeTTL(DirIndex dirIndex, Metadata metadata)
metadata.getJobName(), metadata.getJobId(), metadata.getTaskName(), metadata.getStoreName());
List<FileBlob> fileBlobs = file.getBlobs();
for (FileBlob fileBlob : fileBlobs) {
- String opname = "removeTTL for fileBlob: " + file.getFileName() + " with blobId: {}" + fileBlob.getBlobId();
- Supplier<CompletionStage<Void>> ttlRemovalAction = () ->
- blobStoreManager.removeTTL(fileBlob.getBlobId(), requestMetadata).toCompletableFuture();
- CompletableFuture<Void> ttlRemovalFuture =
- FutureUtil.executeAsyncWithRetries(opname, ttlRemovalAction, isCauseNonRetriable(), executor, retryPolicyConfig);
- updateTTLsFuture.add(ttlRemovalFuture);
+ if (ttlRemovedBlobIdsCache.getIfPresent(fileBlob.getBlobId()) == null) {
+ String opname = "removeTTL for fileBlob: " + file.getFileName() + " with blobId: {}" + fileBlob.getBlobId();
+ Supplier<CompletionStage<Void>> ttlRemovalAction = () ->
+ blobStoreManager.removeTTL(fileBlob.getBlobId(), requestMetadata).toCompletableFuture()
+ .thenRun(() -> ttlRemovedBlobIdsCache.put(fileBlob.getBlobId(), new Object()));
Review Comment:
Thanks. Added timestamp that to the cache and to debug lines.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org