[WIP]Fix bug for ttl smaller than age

[cdyangzhenyu]

Redmine: http://192.168.15.2/issues/11180

[zh-f]

LGTM

[zhaochao]

看起来 message 有 expires 和 ttl 2个属性，一个是具体的过期时刻，一个是过期时间跨度，本身作用是相同的，真正起作用的是 expires ，ttl 不用于自动删除过期消息。

而在代码里，如果没有修改 message ttl 的话，这个属性不会更新，也就是如果一开始设置的是 60 ，后面应该一直是 60 ，只是 message expires 会一直刷新 。Redmine issue 里的问题其实是用 claim ttl 替换了 message ttl ，但是看代码的设计是在 claim 时，如果 claim 导致过期时刻推迟，则会用 claim ttl 替换 message ttl 。这样从逻辑上看，没有特别的错误。

当然在展示时会出现 age 大于 ttl 的确不符合直觉，但是实际上消息被更新过，所以过期的具体时刻会被推迟，从而出现 age 大于的 ttl。如果需要修改 message ttl 的话，这里应该也不能增加 message_ttl ，应该是增加 message_expiration - message 原来的 expires (或者说 message_expiration - meesage 的创建时间）。

另外，如果要改 message ttl 的话，看起来还有别的地方要改，比如这个文件下面的 update 方法，也更新了 message_expiration ，这样 message ttl 也要相应修改。

[huntxu]

我的意见类似，一样不明白这里ttl的意义和作用。

如果ttl的作用和意义如上面 @zhaochao 的评论，那么它是一个字面意义上的TTL的值，表示的是一个消息距离过期还有多长时间。所以应该有类似的等式成立：

1. 过期时间 - 创建时间 = Age + TTL
2. 当前时间 = 创建时间 + Age = 过期时间 - TTL

如果它指的不是过期时间，而是下次可供消费的时间，那么把上面的等式中的过期时间替换成"下次可消费时间"也是一样成立的。

因此TTL应该和Age一样是个动态的值。过期时间不变，Age增加，则TTL减小；过期时间变化，则TTL根据计算得出。TTL的值比Age小，只说明消息的存活(或者whatever)的时间过半，而没有其他的问题。为什么TTL比Age小会是一个bug？这两个并不是可以比较的值。

回到问题本身，上面说的这里的问题是用claim ttl替换了message ttl。所以如果这种替换是在预期中的则没有问题；如果计算的时候需要以message ttl为准，则有需要修改。

[cdyangzhenyu]

@huntxu @zhaochao 这里的TTL是指该消息总的存活时间，不是距当前还可存活时间，age是当前已经存活时间，而真正的控制什么时候删除是用message expiration这个参数，该参数也是根据TTL计算得到的。正常情况下TTL就是应该大于AGE的。这里的问题是当CLAIM TTL+ AGE > TTL的时候，把消息的TTL延长一个CLAIM TTL的时间，相应的expiration也延长一个CLAIM TTL时间。但是这个TTL是可以供查询的，这个值虽然不是实际控制消息删除的值，但是TTL突然变小应该是有问题的，这个社区已经确认是bug了。但是准确的改法确实应该如赵超所说，新的TTL=CLAIM TTL+AGE，但是这个AGE是消息拿出来之后动态计算的，就是expiration减去create time，所以这里没这样做，毕竟这样做很耗费资源。所以这里折中解决，让TTL>AGE。
下面的update方法确实也要修改，但是由于我们产品需求里没有涉及到这个方法，所以之前一直没有关注过这个方法。

[huntxu]

除非这里的TTL有第二个缩写意思或者这里用错了变量名，否则不可能使用TTL(Time To Live)来表示一个总时间，只会表示一个剩余时间。

这里原来的update操作过后，claim meta里（即c里面）t的值是ttl，e的值是now+ttl；消息（即整个row)里t的值是ttl+grace，e的值是now+ttl+grace；两者之间的区别都是一个grace的大小，并没有看出什么问题，也没有看出来ttl必须比age大的原因。

@cdyangzhenyu 上游确认的issue能否贴出链接？

[cdyangzhenyu]

可是在程序里不会去动态修改TTL的属性的，在正常情况下去查看，这个值是不会变的。这里的到期时间只能理解为创建的那一刻的TIME TO LIVE，不是时时刻刻的。当然也有可能社区对这一块的理解有问题，这个是另外的问题。我需要和社区沟通一下。是否需要改为时时刻刻能够体现消息的TIME TO LIVE。
如果这里的TTL是动态变得，那这里是没有问题的。比如每次查询消息的时候，ttl都在逐渐递减。但其实不是这样的。
这里的TTL更像是消息的生命周期的概念，就是一个总的存活时间，只能说这个TTL的命名不是很规范。而我们产品里的定义也是生命周期。
这个是我提的bug，目前已经确认为Medium级别。
https://bugs.launchpad.net/zaqar/+bug/1730335
@huntxu

[cdyangzhenyu]

就是在代码的实现里，已经把TTL当做生命周期在使用了。这里不能通过正常的概念去看待这个参数。

[cdyangzhenyu]

@huntxu 刚查了下，TTL好像一直都是表示生命周期吧，没说是个剩余时间啊。
网络里的TTL概念好像会减少，每经过一个路由器会减1吧。
看了redis的ttl概念，貌似也是总的生命周期的意思，跟zaqar的一样。

[huntxu]

@cdyangzhenyu

因为它把ttl直接给写到数据库中，参考我下面的评论，我也倾向认为这里它是把这个值认为是一个不变的总生命长度，因为一个可变的ttl不太应该写入数据库中(除非弄错了)。或者如你所说，它只能表示创建那一刻的Time To Live，这样子其实意义也不大。

至于这里关于TTL的命名，通常情况下TTL都应该是个可变值而不是固定值，根据时间或者(网络协议上)跳数递减；因此我是没有见过拿TTL表示总时间的例子。关于用法没有看到有特别的限制，但是中间的to表示未来将发生应该没有疑问。参考redis，以及网络hop之类的做法，ttl通常表达的是一个会随着某种其他因素而递减的可变值。

[cdyangzhenyu]

那这个TTL的概念可能是zaqar使用的有问题了。目前确实是拿来当做总的生命周期来使用的。我跟社区建议一下。

[huntxu]

see inline comment

[huntxu]

我的意见类似，一样不明白这里ttl的意义和作用。

如果ttl的作用和意义如上面 @zhaochao 的评论，那么它是一个字面意义上的TTL的值，表示的是一个消息距离过期还有多长时间。所以应该有类似的等式成立：

1. 过期时间 - 创建时间 = Age + TTL
2. 当前时间 = 创建时间 + Age = 过期时间 - TTL

如果它指的不是过期时间，而是下次可供消费的时间，那么把上面的等式中的过期时间替换成"下次可消费时间"也是一样成立的。

因此TTL应该和Age一样是个动态的值。过期时间不变，Age增加，则TTL减小；过期时间变化，则TTL根据计算得出。TTL的值比Age小，只说明消息的存活(或者whatever)的时间过半，而没有其他的问题。为什么TTL比Age小会是一个bug？这两个并不是可以比较的值。

回到问题本身，上面说的这里的问题是用claim ttl替换了message ttl。所以如果这种替换是在预期中的则没有问题；如果计算的时候需要以message ttl为准，则有需要修改。

[cdyangzhenyu]

另外看了社区关于这一块的单元测试，确定当时考虑的太简单了。只是考虑了在开始的时候的情况，比如刚开始消息的ttl是120，第一次消费claim ttl就是200，那这个时候直接把ttl改成200，好像也说的过去，毕竟age还是0，但是如果当age是100，claim ttl是30的时候，那ttl直接就变成了30，这与ttl和age的定义是不符合的。

[huntxu]

继续看了代码，其实这里不理解的是claim_ttl的具体作用，是否指每次claim需要为消息增加更多的存活时间？

因为看原来的代码claim的t和message的t的值，在每次claim的操作都是不变的，恒等于claim_ttl和claim_ttl + claim_grace。但是每次claim，会将claim_expires和message_expiration分别设置为当前时间加上前面两个计算出来的值，亦即把超时时间设置成一个新值。

所以在claim完成的瞬间，在这段代码里ttl是符合字面意思的，因为两个超时时间都是根据当前时间加上ttl算出来的。距离下次claim还有claim_ttl时间，距离消息超时还有claim_ttl+claim_grace时间。

如果我们要把ttl认为是总共能够存活时间，那么原来这部分代码对这两个ttl的使用和更新就都有问题了，的确需要像 @zhaochao 说的那样，相当于age + claim_ttl+claim_grace，而不能只单独增加一个claim_ttl，否则输出的消息详情中各种时间会对应不上。

总之要么ttl = 过期时间 - 创建时间，要么ttl + age = 过期时间 - 创建时间。单独给ttl增加一个claim_ttl的值并不能满足前者的等式。所以这里的方向应该类似如下：

1. 如果message的ttl表示的是总时间，那么这里的增加应该将它改成age+claim_ttl+claim_grace，或者其实等于expiration_time - creation_time；当然，这种思路的话，这个栏位的命名是否能叫做ttl我是持保留意见的。
2. 如果message的ttl表示的是未来的存活时间，那么这里变量的命名不需要考虑，但ttl的输出应该是可以可变值，需要在输出的时候根据当前时间和过期时间来计算。这种思路的话，message中保存的ttl的值，应该只能表示它默认的ttl的值，而不能认为是实际上的ttl。

当然这里还有思路三，那就是在消息本身已经给了生命长度的情况下，claim操作不去影响消息的ttl和到期时间。

[cdyangzhenyu]

@huntxu 这里就是为了避免claim还没有结束，消息已经被删除了。这属于产品或者社区对zaqar的一种定义吧。因为在claim的过程中，可以通过get请求去观察消息的状态。注意这里的条件是，只有当消息的claim ttl + age > message ttl的时候，才会做这种处理，通过这个条件保证：'e': {'$lt': claim_expires_dt},。
前面也说了，正确的做法确实是，新的ttl=claim ttl + age，但是这里获取不到age，除非把每个消息在读取一次，然后每一个都计算一下，这个是很耗费资源的。我觉得没这个必要，只要能保证age<=ttl（age：消息年龄，ttl：生命周期，消息总的存活时间）就可以了，至少从概念上不会有冲突，这样不会迷惑到用户，而具体消息什么时候删除的，一般用户也不会去关心这个吧。所以我说这只能是个折中的改法。

[cdyangzhenyu]

@huntxu 另外claim ttl是指claim这个操作的存活时间，翻译成消息里的术语就是用户消费一个消息的时候，可以保证独自占用这个消息的时间，在这个时间内，其他用户是无法消费这个消息的。而grace是指在这个时间的基础上，又宽限一定的时间。

[huntxu]

前面也说了，正确的做法确实是，新的ttl=claim ttl + age，但是这里获取不到age，除非把每个消息在读取一次，然后每一个都计算一下，这个是很耗费资源的。我觉得没这个必要，只要能保证age<=ttl（age：消息年龄，ttl：生命周期，消息总的存活时间）就可以了，至少从概念上不会有冲突，这样不会迷惑到用户，而具体消息什么时候删除的，一般用户也不会去关心这个吧。所以我说这只能是个折中的改法。

这里如果只增加ttl值的话会有不小的问题，因为每次ttl的增加都比实际上需要增加的值多了一个ttl - age的误差，除非每次都是在刚好将要过期时去claim。因此多次claim发生之后，会造成TTL - Age远大于expiration_time - creation_time的情况，也就是消息的生存时间远小于它的可存在时间，但却即将过期；如果类似的误差能接受，那还不如放弃这个修改，在每次get的时候动态计算ttl的值，将TTL认为是一个即将到期时间，这样子至少能保证输出的内容没有明显漏洞。

至于所说的性能问题，一来这里已经有一个循环存在，二来不需要每个消息获得age，上面也说过了，如果将TTL认为是总时间，那么它等于expiration_time - creation_time，这两个变量应该都是现成的。

[cdyangzhenyu]

@huntxu 这里只可能有一次，不可能有多次，因为改过一次之后，消息在本次claim ttl到期的时候，它自己真实的存活时间也就到期了（这里是用的expires这个控制的，而ttl只是展示作用），此时消息会被删除。所以说不会出现你说的这种情况的。这里唯一的问题就是，消息被删除的时间比实际上展示的ttl时间早了一些。

[huntxu]

@cdyangzhenyu 如果grace>0，甚至远大于0，会不会存在被再次claim和延后expiration_time的可能，即它已经脱离了claim的状态，但还没过期，这种情况如何保证它不被再次claim？

另外，看起来这个ttl并不太需要被保留在数据库中，如上游bug的评论，它唯一的作用似乎只是创建时用来计算expiration_time。所以看起来我们可以甚至不关注这个message_ttl的值(当然claim的ttl是有用的)，反而在get的时候使用expiration_time和creation_time来计算总共的可存活时间，这样子避免各种误差的可能，也能保证总数据一致。但我不知道message的ttl这个值会不会被发送到客户端，如果会的话，似乎只能在这里保证数据库里的值总是正确更好。但如果只有get操作涉及ttl值而get操作(查看消息状态)又不是一个常用操作的话，应该调整get操作的返回更好。

[zhaochao]

比较赞同 @huntxu 后面的意见，即不关注 message ttl ，只修改 client （通过 expires - created_at ）来提示用户消息的最大可存活时间。

[cdyangzhenyu]

@huntxu @zhaochao ttl这个是消息的一个属性（可以发送消息的时候设置，每个消息都可以不同，不设置的话默认使用队列的ttl，这个定义就是消息的生命周期），在消费和get的时候都是要返回给client的。至于是否应该使用剩余可存活时间，这个要看产品的定义了，目前产品对这里定义的就是生命周期，不是剩余可存活时间。另外如果要看剩余可存活时间是通过age来看的，也就是用这里的ttl-age，所以这里不能让age>ttl。我觉的这里没有必要纠结ttl只能用来表示剩余存活时间这个概念，这只是一个变量而已，只能说zaqar在定义这个变量的时候欠考虑。我们还是要遵循代码逻辑里想表达的意思以及产品的定义。

另外grace是指的claim这个操作的宽限时间，所以判断claim有没有过期，是加上了grace这个时间的，也就是说判断claim有没有过期是用的c.e而不是c.t，所以你前面提到的grace>0，或者很大的时候，在claim没有过期的时候，也不会被再次claim。

[huntxu]

@cdyangzhenyu 如果不会被再次claim且message的ttl值是有必要展现的话，那这里还是应该计算之后给出一个正确值。

另外c.e = now + ttl，message的e才加上了grace，grace > 0则message.e大于message.c.e，这样确定没可能出现claim过期但message没过期的场景吗？

[zhaochao]

@cdyangzhenyu
根据前面的讨论，可以认为 message 本身的 ttl 其实是没有错的，按照生命周期的解释也是一样，因为在 claim 时消息被更新过了，所以 ttl 以当时为准。

之所以说在 client 端修改，是为了避免误解，从而通过 expires - created_at 来告诉用户这条消息一共可以存活这么久（从创建到最终过期被删除）。而且实际在数据库里用 ttl 来保存这个数值的意义也不是很明显，因为 expires - created_at 总是正确的。这样改动之后，只是说客户端拿到的这个 ttl 不直接对应数据库里 message 的 ttl 属性而已。

[huntxu]

@zhaochao 因为消费消息也需要给出ttl的值，所以如果要在终端计算的话，涉及的地方可能包括get和消费等不止一个操作；所以如果涉及的地方多的话，还是保证数据库里的值总是正确的好点。(当然数据库里该不该存这个值其实另说...)

[zhaochao]

@huntxu
嗯，也是个问题，要存在数据库还是应该保存正确的值。不过 api 或者 client 正常应该把 message 单独抽象成一个对象了，在那里统一改一次应该是可行的。

[cdyangzhenyu]

@huntxu @zhaochao 这个问题我和产品沟通了一下， 目前我们的产品设计的时候是不需要消息的TTL以及AGE的，也就是上层调用接口的时候也不会关心这些属性。鉴于这里只是显示会有些歧义，所以决定暂不修改。
但是这个问题我还会继续和社区沟通，看看社区的意见。

看了讨论，瑟瑟发抖……