User disabling `AppendBrokerTimestampMetadataInterceptor` does not mean that they allow this bug. This is a configuration, not an API. It is difficult to use documentation to regulate user behavior.
Maybe we can add a new field (BrokerTimestamp) to save the timestamp on the Broker side. The time priority for trimming Ledger is as follows: BrokerPublishTime > BrokerTimestamp If `BrokerPublishTime` exists, `BrokerReceiveTime` is not set. If not, we set `BrokerReceiveTime` and is no longer affected by client time. On 2024/01/15 02:15:17 PengHui Li wrote: > IMO, we should enable `AppendBrokerTimestampMetadataInterceptor` by default. > Users can still have a way to disable it if they care about the additional > metadata stored > in each entry. > > For the `hasBrokerPublishTime` API. The topic might also have historical > data without > broker publish time. So, it should be fine to keep this API because we > don't know how > long users will retain their data. > > Regards, > Penghui > > On Sat, Jan 6, 2024 at 10:35 PM linlin <lin...@apache.org> wrote: > > > Now, if the message's metadata does not set a broker side timestamp, the > > ledger expiration check is based on the client's publish time. > > > > When the client machine's clock is incorrect (eg: set to 1 year later) , > > the ledger can not be cleaned up. Issue > > https://github.com/apache/pulsar/issues/21347 > > > > `AppendBrokerTimestampMetadataInterceptor` can set timestamp for messages > > on the broker side, but we can not ensure that the > > `AppendBrokerTimestampMetadataInterceptor` is always enable > > > > Therefore, I open this PR(https://github.com/apache/pulsar/pull/21835) to > > always set the broker timestamp for messages on the broker side. > > > > With this change , firstly we should deprecate > > AppendBrokerTimestampMetadataInterceptor. > > It no longer needs to exist > > > > Secondly, we should deprecate `hasBrokerPublishTime` in interface Message. > > It always returns true. > > This API is created in PR (https://github.com/apache/pulsar/pull/11553) > > This PR is for the client to obtain BrokerPublishTime, so the > > `hasBrokerPublishTime` API is not necessary. > > >
