You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "315157973 (via GitHub)" <gi...@apache.org> on 2024/01/02 12:27:39 UTC
[PR] Fix messages could not expire due to incorrect client clock [pulsar]
315157973 opened a new pull request, #21835:
URL: https://github.com/apache/pulsar/pull/21835
Fixes #21347
### Motivation
When the client machine's clock is incorrect (eg: set to 1 year later) and the Broker does not set the AppendBrokerTimestampMetadataInterceptor, the Ledger will not be cleaned up.
Because if the Broker's timestamp is not set, the expiration check will be based on the client's sending time.
### Modifications
Always set the Broker's timestamp
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#issuecomment-1874083823
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21835?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`8ff32db`)](https://app.codecov.io/gh/apache/pulsar/commit/8ff32dbf9eb63de3554ed546703030d4345b6d27?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.63% compared to head [(`33e9158`)](https://app.codecov.io/gh/apache/pulsar/pull/21835?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.61%.
> Report is 6 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/21835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #21835 +/- ##
============================================
- Coverage 73.63% 73.61% -0.03%
- Complexity 32288 32316 +28
============================================
Files 1858 1858
Lines 138098 138146 +48
Branches 15135 15142 +7
============================================
+ Hits 101688 101691 +3
- Misses 28552 28597 +45
Partials 7858 7858
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21835/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.19% <0.00%> (-0.06%)` | :arrow_down: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/21835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.69% <0.00%> (-0.04%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.90% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pulsar/pull/21835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pulsar/common/protocol/Commands.java](https://app.codecov.io/gh/apache/pulsar/pull/21835?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9wcm90b2NvbC9Db21tYW5kcy5qYXZh) | `90.71% <100.00%> (+0.01%)` | :arrow_up: |
... and [78 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21835/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#discussion_r1440290158
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java:
##########
@@ -1697,13 +1697,16 @@ public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
// | BROKER_ENTRY_METADATA_MAGIC_NUMBER | BROKER_ENTRY_METADATA_SIZE | BROKER_ENTRY_METADATA |
// | 2 bytes | 4 bytes | BROKER_ENTRY_METADATA_SIZE bytes |
- BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get();
+ BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get().clearBrokerTimestamp();
Review Comment:
Please call `.clear()` to reset all state.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#discussion_r1441380982
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java:
##########
@@ -1697,13 +1697,16 @@ public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
// | BROKER_ENTRY_METADATA_MAGIC_NUMBER | BROKER_ENTRY_METADATA_SIZE | BROKER_ENTRY_METADATA |
// | 2 bytes | 4 bytes | BROKER_ENTRY_METADATA_SIZE bytes |
- BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get();
+ BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get().clearBrokerTimestamp();
for (BrokerEntryMetadataInterceptor interceptor : brokerInterceptors) {
interceptor.intercept(brokerEntryMetadata);
if (numberOfMessages >= 0) {
interceptor.interceptWithNumberOfMessages(brokerEntryMetadata, numberOfMessages);
}
}
+ if (!brokerEntryMetadata.hasBrokerTimestamp()) {
+ brokerEntryMetadata.setBrokerTimestamp(System.currentTimeMillis());
+ }
Review Comment:
Unless we can ensure that the `AppendBrokerTimestampMetadataInterceptor` cannot be disabled, this bug may always occur
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#discussion_r1441266759
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java:
##########
@@ -1697,13 +1697,16 @@ public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
// | BROKER_ENTRY_METADATA_MAGIC_NUMBER | BROKER_ENTRY_METADATA_SIZE | BROKER_ENTRY_METADATA |
// | 2 bytes | 4 bytes | BROKER_ENTRY_METADATA_SIZE bytes |
- BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get();
+ BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get().clearBrokerTimestamp();
for (BrokerEntryMetadataInterceptor interceptor : brokerInterceptors) {
interceptor.intercept(brokerEntryMetadata);
if (numberOfMessages >= 0) {
interceptor.interceptWithNumberOfMessages(brokerEntryMetadata, numberOfMessages);
}
}
+ if (!brokerEntryMetadata.hasBrokerTimestamp()) {
+ brokerEntryMetadata.setBrokerTimestamp(System.currentTimeMillis());
+ }
Review Comment:
The broker timestamp will be set in line:1702 if the `AppendBrokerTimestampMetadataInterceptor` is enabled. Is this PR wants to set the broker timestamp without `AppendBrokerTimestampMetadataInterceptor` enabled?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#discussion_r1442432819
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java:
##########
@@ -1697,13 +1697,16 @@ public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
// | BROKER_ENTRY_METADATA_MAGIC_NUMBER | BROKER_ENTRY_METADATA_SIZE | BROKER_ENTRY_METADATA |
// | 2 bytes | 4 bytes | BROKER_ENTRY_METADATA_SIZE bytes |
- BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get();
+ BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get().clearBrokerTimestamp();
for (BrokerEntryMetadataInterceptor interceptor : brokerInterceptors) {
interceptor.intercept(brokerEntryMetadata);
if (numberOfMessages >= 0) {
interceptor.interceptWithNumberOfMessages(brokerEntryMetadata, numberOfMessages);
}
}
+ if (!brokerEntryMetadata.hasBrokerTimestamp()) {
+ brokerEntryMetadata.setBrokerTimestamp(System.currentTimeMillis());
+ }
Review Comment:
Ok, I think we can either make the AppendBrokerTimestampMetadataInterceptor enabled by default or remove the AppendBrokerTimestampMetadataInterceptor to always apply the broker timestamp.
Without this change, the broker timestamp will be enabled without AppendBrokerTimestampMetadataInterceptor. Essentially, we delete the functionality of the AppendBrokerTimestampMetadataInterceptor.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#issuecomment-2288957331
As https://github.com/apache/pulsar/pull/21940 is merged, close this PR
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on code in PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#discussion_r1444517029
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java:
##########
@@ -1697,13 +1697,16 @@ public static ByteBuf addBrokerEntryMetadata(ByteBuf headerAndPayload,
// | BROKER_ENTRY_METADATA_MAGIC_NUMBER | BROKER_ENTRY_METADATA_SIZE | BROKER_ENTRY_METADATA |
// | 2 bytes | 4 bytes | BROKER_ENTRY_METADATA_SIZE bytes |
- BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get();
+ BrokerEntryMetadata brokerEntryMetadata = BROKER_ENTRY_METADATA.get().clearBrokerTimestamp();
for (BrokerEntryMetadataInterceptor interceptor : brokerInterceptors) {
interceptor.intercept(brokerEntryMetadata);
if (numberOfMessages >= 0) {
interceptor.interceptWithNumberOfMessages(brokerEntryMetadata, numberOfMessages);
}
}
+ if (!brokerEntryMetadata.hasBrokerTimestamp()) {
+ brokerEntryMetadata.setBrokerTimestamp(System.currentTimeMillis());
+ }
Review Comment:
I opened a discuss in dev thread https://lists.apache.org/thread/0vtjt436xvlg0fr4ogchkjdvjcp56mvz
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 commented on PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#issuecomment-2288950920
@lhotari @codelipenghui Can this PR be merged?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "315157973 (via GitHub)" <gi...@apache.org>.
315157973 closed pull request #21835: [fix][broker] Fix messages could not expire due to incorrect client clock
URL: https://github.com/apache/pulsar/pull/21835
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21835:
URL: https://github.com/apache/pulsar/pull/21835#issuecomment-1874157322
Please also update the comment in `org.apache.pulsar.common.protocol.Commands#getEntryTimestamp` . I guess that now the BrokerEntryMetadata with the timestamp will always be added?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix messages could not expire due to incorrect client clock [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #21835: [fix][broker] Fix messages could not expire due to incorrect client clock
URL: https://github.com/apache/pulsar/pull/21835
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org