szetszwo commented on a change in pull request #2296:
URL: https://github.com/apache/hadoop/pull/2296#discussion_r486594478
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
##########
@@ -448,22 +455,31 @@ public String createSnapshot(final LeaseManager
leaseManager,
"snapshot IDs and ID rollover is not supported.");
}
int n = numSnapshots.get();
- if (n >= maxSnapshotFSLimit) {
- // We have reached the maximum snapshot limit
- throw new SnapshotException(
- "Failed to create snapshot: there are already " + (n + 1)
- + " snapshot(s) and the max snapshot limit is "
- + maxSnapshotFSLimit);
- }
-
- srcRoot.addSnapshot(snapshotCounter, snapshotName, leaseManager,
- this.captureOpenFiles, maxSnapshotLimit, mtime);
+ checkSnapshotLimit(maxSnapshotFSLimit, n);
+ srcRoot.addSnapshot(this, snapshotName, leaseManager, mtime);
//create success, update id
snapshotCounter++;
numSnapshots.getAndIncrement();
return Snapshot.getSnapshotPath(snapshotRoot, snapshotName);
}
+
+ void checkSnapshotLimit(int limit, int numSnapshots)
Review comment:
I suggest to add limit type to the error message as below.
```
diff --git
a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
index 266c0a71241..7a47ab4000d 100644
---
a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
+++
b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
@@ -190,8 +190,7 @@ public Snapshot addSnapshot(INodeDirectory snapshotRoot,
+ n + " snapshot(s) and the snapshot quota is "
+ snapshotQuota);
}
- snapshotManager.checkSnapshotLimit(snapshotManager.
- getMaxSnapshotLimit(), n);
+ snapshotManager.checkPerDirectorySnapshotLimit(n);
final Snapshot s = new Snapshot(id, name, snapshotRoot);
final byte[] nameBytes = s.getRoot().getLocalNameBytes();
final int i = searchSnapshot(nameBytes);
diff --git
a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
index 0a2e18c3dc3..7c482074486 100644
---
a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
+++
b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
@@ -455,7 +455,7 @@ public String createSnapshot(final LeaseManager
leaseManager,
"snapshot IDs and ID rollover is not supported.");
}
int n = numSnapshots.get();
- checkSnapshotLimit(maxSnapshotFSLimit, n);
+ checkFileSystemSnapshotLimit(n);
srcRoot.addSnapshot(this, snapshotName, leaseManager, mtime);
//create success, update id
@@ -464,12 +464,19 @@ public String createSnapshot(final LeaseManager
leaseManager,
return Snapshot.getSnapshotPath(snapshotRoot, snapshotName);
}
- void checkSnapshotLimit(int limit, int numSnapshots)
- throws SnapshotException {
+ void checkFileSystemSnapshotLimit(int n) throws SnapshotException {
+ checkSnapshotLimit(maxSnapshotFSLimit, n, "file system");
+ }
+
+ void checkPerDirectorySnapshotLimit(int n) throws SnapshotException {
+ checkSnapshotLimit(maxSnapshotLimit, n, "per directory");
+ }
+
+ private void checkSnapshotLimit(int limit, int numSnapshots,
+ String type) throws SnapshotException {
if (numSnapshots >= limit) {
- String msg = "there are already " + (numSnapshots + 1)
- + " snapshot(s) and the max snapshot limit is "
- + limit;
+ String msg = "There are already " + (numSnapshots + 1)
+ + " snapshot(s) and the " + type + " snapshot limit is " + limit;
if (fsdir.isImageLoaded()) {
// We have reached the maximum snapshot limit
throw new SnapshotException(
```
----------------------------------------------------------------
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.
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
