-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDFS-15201 SnapshotCounter hits MaxSnapshotID limit #1870
Conversation
...doop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
Submitted new PR with the changes requested. |
💔 -1 overall
This message was automatically generated. |
...-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotManager.java
Show resolved
Hide resolved
@karthikhw can you double check the failed tests? especially the snapshot tests. |
I've gone through all the usage of the snapshot id. The only concern i had was bitwise operations on the snapshot id, but i didn't find any. Widening the allowed range shouldn't be a problem. |
...doop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
...doop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
It looks test case failure is un-related to this issue. |
@arp7 @jojochuang Can you please review this change if you get sometime? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@szetszwo do you want to take a look too?
The changes look good. Just a question: why changing getMaxSnapshotID() from -1 to -2? |
Changed getMaxSnapshotID() from -1 to -2 because of CURRENT_STATE_ID(Integer.MAX_VALUE - 1) that have -1 already. If SNAPSHOT_ID_BIT_WIDTH is 28 then we are ok with -1 but later change in SNAPSHOT_ID_BIT_WIDTH to 31 then user have to update getMaxSnapshotID() from -1 to -2. |
Let's keep it "-1" since we are using 28 for the moment. If there still a problem later on, we should think about what to do. We do not necessarily change it to 31 at that time. |
Thank you @szetszwo I changed back to -1. |
+1 the latest change looks good. Thanks @karthikhw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karthikhw Thanks for working on this! The changes look good to me. +1.
@karthikhw Thanks for the contribution! @jojochuang @mukul1987 @arp7 @szetszwo Thanks for reviewing the PR! I have committed it to master branch. |
At the beginning, SNAPSHOT_ID_BIT_WIDTH was setted 31, but later was setted 28. Why change SNAPSHOT_ID_BIT_WIDTH from 31 to 28? I think maybe:
|
(cherry picked from commit 5250cd6) Change-Id: Ibf48916c28f35e866d8b441af65de1a0b92b1733 (cherry picked from commit 20ea94d4a940cef35f0ff873dfaea19c6e5a7b83)
Jira: https://issues.apache.org/jira/browse/HDFS-15201
Users reported that they are unable to take HDFS snapshots and their snapshotCounter hits MaxSnapshotID limit. MaxSnapshotID limit is 16777215.
I think, SNAPSHOT_ID_BIT_WIDTH is too low. May be good idea to increase SNAPSHOT_ID_BIT_WIDTH to 31? to aline with our CURRENT_STATE_ID limit (Integer.MAX_VALUE - 1).