ayushtkn commented on code in PR #5809:
URL: https://github.com/apache/hadoop/pull/5809#discussion_r1257856998
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/FSDatasetMBean.java:
##########
@@ -122,4 +122,9 @@ public interface FSDatasetMBean extends MetricsSource {
* Returns the number of blocks that the datanode was unable to uncache
*/
public long getNumBlocksFailedToUncache();
+
+ /**
+ * Returns the last time in milliseconds of directory scanner run.
+ */
Review Comment:
can you change to
```
Return the last time in milliseconds when the directory scanner successfully
ran.
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java:
##########
@@ -487,6 +487,7 @@ public void reconcile() throws IOException {
if (!retainDiffs) {
clear();
}
+ dataset.setLastDirScannerFinishTime(System.currentTimeMillis());
Review Comment:
I think this should be done in the run the method after reconcile, from the
correctness point of view
```
reconcile();
dataset.setLastDirScannerFinishTime(System.currentTimeMillis());
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java:
##########
@@ -692,4 +692,10 @@ ReplicaInfo moveBlockAcrossVolumes(final ExtendedBlock
block,
* Get the volume list.
*/
List<FsVolumeImpl> getVolumeList();
+
+ /**
+ * Set the finish time of last directory scan.
+ * @param time
Review Comment:
add description for the param, something like:
```
* @param time the time when last directory scan finished successfully.
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalDatasetImpl.java:
##########
@@ -53,6 +53,7 @@ public class ExternalDatasetImpl implements
FsDatasetSpi<ExternalVolumeImpl> {
private final DatanodeStorage storage = new DatanodeStorage(
DatanodeStorage.generateUuid(), DatanodeStorage.State.NORMAL,
StorageType.DEFAULT);
+ private long lastDirScannerFinishTime;
Review Comment:
unused?
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalDatasetImpl.java:
##########
@@ -477,4 +478,14 @@ public MountVolumeMap getMountVolumeMap() {
public List<FsVolumeImpl> getVolumeList() {
return null;
}
+
+ @Override
+ public long getLastDirScannerFinishTime() {
+ return 0L;
+ }
+
+ @Override
+ public void setLastDirScannerFinishTime(long time) {
+
+ }
Review Comment:
can we not have default empty implementation in the parent class, rather
than implementing empty methods in child classes?
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
