[GitHub] [hbase] virajjasani edited a comment on pull request #3852: HBASE-26458 Add UNSET_SNAPSHOT_PROP and fix TTL defaulting

[GitBox]

virajjasani edited a comment on pull request #3852:
URL: https://github.com/apache/hbase/pull/3852#issuecomment-971465945


   I just tested on the latest HEAD of master branch.
   
   **Configs:**
   ```
     <property>
       <name>hbase.master.cleaner.snapshot.interval</name>
       <value>10000</value>
     </property>
   ```
   
   **Shell commands and respective outputs:**
   
   > snapshot 't1', 's2'
   Took 2.1034 seconds           
   
   > snapshot 't1', 's3', {TTL => -2}
   Took 1.0925 seconds                                                          
                                                                                
                                                                                
                 
   => [{"TTL"=>-2}]
   
   > snapshot 't1', 's1', {TTL => 5}
   Took 2.1023 seconds                                                          
                                                                                
                                                                                
                 
   => [{"TTL"=>5}]
   
   <img width="1630" alt="Screenshot 2021-11-17 at 4 17 11 PM" 
src="https://user-images.githubusercontent.com/34790606/142187725-e0f67af3-90bc-47ef-bd28-f73285c27dd3.png";>
   
   
   After 5sec, the snapshot was gone.
   
   <img width="1568" alt="Screenshot 2021-11-17 at 4 19 32 PM" 
src="https://user-images.githubusercontent.com/34790606/142187833-749fb7a2-5a31-4956-9f65-b7fb8bda36ee.png";>
   
   
   
   
   **Configs:**
   ```
     <property>
       <name>hbase.master.cleaner.snapshot.interval</name>
       <value>10000</value>
     </property>
     <property>
       <name>hbase.master.snapshot.ttl</name>
       <value>20</value>
     </property>
   ```
   
   **Shell commands and respective outputs:**
   
   > snapshot 't1', 's4'
   Took 2.7291 seconds                                                          
  
   
   Snapshot 's4' gets created with TTL 20 sec because of config 
`hbase.master.snapshot.ttl`:
   
   <img width="1584" alt="Screenshot 2021-11-17 at 4 21 49 PM" 
src="https://user-images.githubusercontent.com/34790606/142188010-15a2abf1-26aa-4553-a803-ec2e96b5803d.png";>
   
   
   After 20s, it was gone:
   
   <img width="1539" alt="Screenshot 2021-11-17 at 4 22 04 PM" 
src="https://user-images.githubusercontent.com/34790606/142188219-1ada2b96-0a64-4f6c-b5f2-711b3abfc8e8.png";>
   
   
   > snapshot 't1', 's5', {TTL => -10}
   Took 2.1079 seconds                                                          
                                                                                
                                                                                
                 
   => [{"TTL"=>-10}]
   
   <img width="1618" alt="Screenshot 2021-11-17 at 4 23 43 PM" 
src="https://user-images.githubusercontent.com/34790606/142188278-ed70511c-bea6-44f8-a3d6-c70c674bbd57.png";>
   
   1.  TTL with negative values < `-1`, will stay forever regardless of any 
server side config changes.
   2. TTL with positive values will be gone after TTL expires.
   3. Snapshot created without specifying TTL will always have TTL value 
represented by config `hbase.master.snapshot.ttl`.
   4. From client side, TTL values `0` or `-1` should not be provided because 
they will be treated as snapshot without TTL (same as point `3` above) and 
hence will represent TTL as per value represented by config 
`hbase.master.snapshot.ttl`.
   
   I think we have doc with details of server side configs 
`hbase.master.cleaner.snapshot.interval` and `hbase.master.snapshot.ttl`. From 
client viewpoint, we should mention above 4 points.
   
   Also, it's bit complex to resolve point `4` completely because in order to 
represent value as default TTL, we will have to either sacrifice the meaning of 
0 or -1 (if not both) in the best case. I think as long as the above mentioned 
points are taken care of, we should not see any TTL related issues.
   Note: updating default value in proto message is considered as incompatible 
change. Even if we want to present only either 0 or 1 as default TTL, we will 
need changes to proto definition and be careful of the issues that could arise 
from rolling upgrade. Given the incompatibility cost, I believe we should 
rather follow the behaviour mentioned in above 4 points :( Thoughts?


-- 
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: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org