HDFS-17068. Datanode should record last directory scan time. #5809
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The result likes below: |
|
@ayushtkn @Hexiaoqiao . Hi, sir. Could you please help me review this PR when you have free time ? Thanks a lot~ |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jul 7, 2023
| /** | ||
| * Returns the last time in milliseconds of directory scanner run. | ||
| */ | ||
| public long getLastDirectoryScannerRunTime(); |
There was a problem hiding this comment.
This name will be little misleading if it is the begin or end time of one loop of DirectoryScanner.
There was a problem hiding this comment.
Sir. have changed to getLastDirectoryScannerFinishTime and other fields have also modified. Thanks for this review.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
xinglin
reviewed
Jul 7, 2023
| @@ -3811,5 +3818,10 @@ void stopAllDataxceiverThreads(FsVolumeImpl volume) { | |||
| public List<FsVolumeImpl> getVolumeList() { | |||
| return volumes.getVolumes(); | |||
| } | |||
|
|
|||
| @Override | |||
| public void updateLastDirScannerFinishTime(long time) { | |||
There was a problem hiding this comment.
nit: can we rename to setLastDirScannerFinishTime, following the getter/setter convention?
There was a problem hiding this comment.
@xinglin Sir, thanks for suggestions. Have changed.
|
|
||
| beanClass.getNumBlocksFailedToUncache()) | ||
| .addGauge(Interns.info("LastDirectoryScannerFinishTime", | ||
| "Last finish time of directory scanner"), |
There was a problem hiding this comment.
"Last finish time of directory scanner" -> "Finish time of the last directory scan".
I would actually prefer "most recent" over "last", but fine with "last".
There was a problem hiding this comment.
@xinglin Sir, Thanks a lot for reviewing. modify "Last finish time of directory scanner" -> "Finish time of the last directory scan". "last" reserved.
|
🎊 +1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17068
branch
3 times, most recently
from
eb1a7bb
to
10a223d
Compare
xinglin
reviewed
Jul 8, 2023
| @@ -477,4 +482,7 @@ public MountVolumeMap getMountVolumeMap() { | |||
| public List<FsVolumeImpl> getVolumeList() { | |||
| return null; | |||
| } | |||
|
|
|||
| @Override | |||
| public void setLastDirScannerFinishTime(long time) {} | |||
There was a problem hiding this comment.
Is there a reason that in one test case, we throw an unsupported exception while in this test class, it is a no-op? can we make both behave the same way?
There was a problem hiding this comment.
@xinglin Hi, sir. I do not know why exacltly, but i think the SimulatedFSDataset is just for simplifing test. if we make both behave the same way, there is no need to have class SimulatedFSDataset. What's your idea？Thanks a lot.
There was a problem hiding this comment.
+1, I am also confused why this behavior is different with ExternalDatasetImpl#setLastDirScannerFinishTime.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17068
branch
3 times, most recently
from
947e1c4
to
9333981
Compare
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jul 9, 2023
...hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java
Outdated
Show resolved
Hide resolved
...-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java
Show resolved
Hide resolved
| @@ -477,4 +482,7 @@ public MountVolumeMap getMountVolumeMap() { | |||
| public List<FsVolumeImpl> getVolumeList() { | |||
| return null; | |||
| } | |||
|
|
|||
| @Override | |||
| public void setLastDirScannerFinishTime(long time) {} | |||
There was a problem hiding this comment.
+1, I am also confused why this behavior is different with ExternalDatasetImpl#setLastDirScannerFinishTime.
xinglin
approved these changes
Jul 9, 2023
| @@ -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; | |||
There was a problem hiding this comment.
A lot of functions in this class are not implemented. I think we should following the same pattern here.
getLastDirScannerFinishTime() -> return 0.
setLastDirScannerFinishTime() -> do nothing.
Below are functions related with Trash.
@Override
public void enableTrash(String bpid) {
}
@Override
public void clearTrash(String bpid) {
}
@Override
public boolean trashEnabled(String bpid) {
return false;
}
SimulatedFSDataset.java has a different implementation for enableTrash(). So, I guess we don't have to enforce the same implementation between SimulatedFSDataset and ExternalDatasetImpl. I am fine with the current change.
@Override
public void enableTrash(String bpid) {
throw new UnsupportedOperationException();
}
cc @Hexiaoqiao
xinglin
suggested changes
Jul 9, 2023
|
💔 -1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17068
branch
from
2b1902e
to
244f7d8
Compare
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Jul 10, 2023
...oject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DirectoryScanner.java
Outdated
Show resolved
Hide resolved
...hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/FsDatasetSpi.java
Outdated
Show resolved
Hide resolved
...hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/FSDatasetMBean.java
Outdated
Show resolved
Hide resolved
...dfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalDatasetImpl.java
Outdated
Show resolved
Hide resolved
...dfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/extdataset/ExternalDatasetImpl.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17068
branch
from
0e26dfd
to
4354b94
Compare
ayushtkn
reviewed
Jul 11, 2023
...s/src/test/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/TestFsDatasetImpl.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17068
branch
from
9111da7
to
b61bb60
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Jul 12, 2023
...t/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDirectoryScanner.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
@Hexiaoqiao any further comments, planning to hold for you a couple of days |
Hexiaoqiao
approved these changes
Jul 14, 2023
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…5809). Contributed by farmmamba. Reviewed-by: He Xiaoqiao <hexiaoqiao@apache.org> Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
please see HDFS-17068.