prateekm commented on code in PR #1612: URL: https://github.com/apache/samza/pull/1612#discussion_r889087899
########## samza-api/src/main/java/org/apache/samza/storage/blobstore/exceptions/BlobNotFoundException.java: ########## @@ -0,0 +1,45 @@ +/* + * 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.samza.storage.blobstore.exceptions; + +/** + * Future should complete with this exception to indicate that the exception occurred due to the request for an + * a blob that is not present. + * + */ +public class BlobNotFoundException extends RuntimeException { Review Comment: NotFoundException to match naming convention for DeletedException etc. ########## samza-core/src/main/java/org/apache/samza/storage/blobstore/BlobStoreBackupManager.java: ########## @@ -122,6 +125,8 @@ public BlobStoreBackupManager(JobModel jobModel, ContainerModel containerModel, this.prevStoreSnapshotIndexesFuture = CompletableFuture.completedFuture(ImmutableMap.of()); this.metrics = blobStoreTaskBackupMetrics; metrics.initStoreMetrics(storesToBackup); + this.blobStoreManagerMetrics = blobStoreTaskManagerMetrics; + blobStoreTaskManagerMetrics.initStoreMetrics(storesToBackup); Review Comment: Why are blob store manager metrics being passed and intialized here instead of in BlobStoreManager init? ########## samza-core/src/main/java/org/apache/samza/storage/blobstore/BlobStoreRestoreManager.java: ########## @@ -107,14 +110,15 @@ public BlobStoreRestoreManager(TaskModel taskModel, ExecutorService restoreExecu this.blobStoreConfig = new BlobStoreConfig(config); this.storageManagerUtil = storageManagerUtil; this.blobStoreManager = blobStoreManager; - this.blobStoreUtil = createBlobStoreUtil(blobStoreManager, executor, metrics); + this.blobStoreUtil = createBlobStoreUtil(blobStoreManager, executor, blobStoreManagerMetrics, metrics); this.dirDiffUtil = new DirDiffUtil(); this.prevStoreSnapshotIndexes = new HashMap<>(); this.loggedBaseDir = loggedBaseDir; this.nonLoggedBaseDir = nonLoggedBaseDir; this.taskName = taskModel.getTaskName().getTaskName(); this.storesToRestore = storesToRestore; - this.metrics = metrics; + this.blobStoreRestoreManagerMetrics = metrics; + this.blobStoreManagerMetrics = blobStoreManagerMetrics; Review Comment: Same as above, why is this updating metrics for an unrelated class? ########## samza-core/src/main/java/org/apache/samza/storage/blobstore/BlobStoreBackupManager.java: ########## @@ -122,6 +125,8 @@ public BlobStoreBackupManager(JobModel jobModel, ContainerModel containerModel, this.prevStoreSnapshotIndexesFuture = CompletableFuture.completedFuture(ImmutableMap.of()); this.metrics = blobStoreTaskBackupMetrics; metrics.initStoreMetrics(storesToBackup); + this.blobStoreManagerMetrics = blobStoreTaskManagerMetrics; + blobStoreTaskManagerMetrics.initStoreMetrics(storesToBackup); Review Comment: Same for all other occurences. ########## samza-core/src/main/java/org/apache/samza/storage/blobstore/metrics/BlobStoreManagerMetrics.java: ########## @@ -0,0 +1,47 @@ +/* + * 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.samza.storage.blobstore.metrics; + +import java.util.Collection; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.samza.metrics.Gauge; +import org.apache.samza.metrics.MetricsRegistry; + + +public class BlobStoreManagerMetrics { + private static final String GROUP = BlobStoreManagerMetrics.class.getName(); + private final MetricsRegistry metricsRegistry; + + public final Map<String, Gauge<Long>> storeBlobNotFoundError; Review Comment: Why is this the only metric handled specially in blob store manager? What about get/put/delete count and timers etc? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
