[GitHub] [hbase] joswiatek commented on pull request #3852: HBASE-26458 Add UNSET_SNAPSHOT_PROP and fix TTL defaulting

[GitBox <gi...@apache.org>]

joswiatek commented on pull request #3852:
URL: https://github.com/apache/hbase/pull/3852#issuecomment-972199303


   Thank you for digging into this! I totally missed the protobuf component. It is great to see this is working on master HEAD.
   
   After reviewing these new protobuf details, I think the behavior where `hbase.master.snapshot.ttl` is ignored is only present on branch-1. On branch-1, the builder just directly takes the value from the snapshotProps, so -1 is actually sent and it does not match `NO_SNAPSHOT_TTL_SPECIFIED`, and the config is not applied. See [here](https://github.com/apache/hbase/blob/0d214ff53e3b8cb3bccbc01815ccbc9a03dfabb4/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java#L3711-L3717).
   ```
       SnapshotDescription.Builder builder = SnapshotDescription.newBuilder();
       builder.setTable(tableName.getNameAsString());
       builder.setName(snapshotName);
       builder.setType(type);
       builder.setTtl(getTtlFromSnapshotProps(snapshotProps));
       snapshot(builder.build());
   ```
   On a branch-1 build, I do see snapshots being created with a -1 TTL when the default config is set.
   #### Configs:
   ```
     <property>
       <name>hbase.master.snapshot.ttl</name>
       <value>600</value>
     </property>
   ```
   
   #### Shell commands and outputs
   ```
   snapshot 't1', 's1'
   ```
   
   Snapshot is actually created with a TTL of `-1` instead of 600
   
   ![image](https://user-images.githubusercontent.com/17076271/142294766-ce998904-3a85-4bf0-850c-99d99601287d.png)
   
   > updating default value in proto message is considered as incompatible change
   
   I am not experienced with protobuf, so I will take your word about defaults here. One question: is changing the logic around building the protobuf also an incompatible change? If we can update the TTL conversion logic to allow for sending `-1`, then the code changes in this PR will make sense and allow for the currently documented behavior to be fulfilled. 
   
   If that's not possible, I can update this PR with the clarifying doc changes as suggested.


-- 
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-unsubscribe@hbase.apache.org

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