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  > 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
