Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix publish_time not set error when broker entry metadata enable without AppendBrokerTimestampMetadataInterceptor #11014

Merged
merged 5 commits into from Jul 4, 2021

Conversation

aloyszhang
Copy link
Contributor

@aloyszhang aloyszhang commented Jun 22, 2021

Fixes #11013

Motivation

fix publish_time not set error when broker entry metadata enable without AppendBrokerTimestampMetadataInterceptor

Modifications

  1. add a new method named getEntryTimestamp which will return the brokerEntryTimestamp if BrokerEntryMetadata is enabled or otherwise return the publishTime.
  2. using this entryTimestamp for expiry checking.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: ( no)
  • The schema: ( no )
  • The default values of configurations: ( no)
  • The wire protocol: ( no)
  • The rest endpoints: ( no)
  • The admin cli options: ( no)
  • Anything that affects deployment: (no)

@BewareMyPower
Copy link
Contributor

After discussing with @aloyszhang , we need to find why the required publish_time field is missed first.

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

publish_time is required filed in message metadata proto, we should figure out the reason why this filed missed first.

@aloyszhang aloyszhang changed the title fix deserialized error fix publish_time not set error when broker entry metadata enable without AppendBrokerTimestampMetadataInterceptor Jun 24, 2021
@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@BewareMyPower
Copy link
Contributor

@lhotari Hi could you help look at this test failure?

Error:  Tests run: 14, Failures: 1, Errors: 0, Skipped: 13, Time elapsed: 57.774 s <<< FAILURE! - in org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListenerTest
Error:  testResourceGroupAttachToNamespace(org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListenerTest)  Time elapsed: 10.167 s  <<< FAILURE!
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a lambda expression in org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListenerTest expected object to not be null within 10 seconds.

I see your PR (#11048) fixed the flaky test, but it still failed for multiple times.

@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@aloyszhang
Copy link
Contributor Author

@BewareMyPower @hangc0276 PTAL

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to rename deserializeBrokerEntryMetadataFirst to deserializeBrokerTimestampFirst. When this method was added, there was only one type of BrokerEntryMetadata: BrokerTimestamp. So it was named to deserializeBrokerMetadata.

However, now we have multiple types of BrokerEntryMetadata. Checking hasBrokerTimestamp() in a deserializeBrokerTimestampFirst method is weird.

@aloyszhang
Copy link
Contributor Author

@BewareMyPower I have added a new method named getEntryTimestamp which will return the brokerEntryTimestamp if BrokerEntryMetadata is enabled or otherwise return the publishTime. And then using this entryTimestamp for expiry checking.

@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

…out AppendBrokerTimestampMetadataInterceptor
@BewareMyPower
Copy link
Contributor

@hangc0276 Could you also take a look?

@aloyszhang
Copy link
Contributor Author

cc @hangc0276

@aloyszhang
Copy link
Contributor Author

@sijie @codelipenghui PTAL

@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@aloyszhang
Copy link
Contributor Author

aloyszhang commented Jul 2, 2021

@hangc0276 Thanks for your comments, I've resolved them.
PTAL again.

@aloyszhang
Copy link
Contributor Author

@hangc0276 PTAL

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BewareMyPower BewareMyPower merged commit fe7cf67 into apache:master Jul 4, 2021
tuteng pushed a commit that referenced this pull request Jul 6, 2021
### Motivation

#11014 introduced `getEntryTimestamp` and removed `deserializeBrokerEntryMetaDataFirst`, but #11139 is still using `deserializeBrokerEntryMetaDataFirst`, and it breaks master branch.

### Modifications

use `entryTimestamp` for expiry checking in `internalGetMessageIdByTimestamp`

### Verifying this change

- [x] Make sure that the change passes the CI checks.
@aloyszhang aloyszhang deleted the proto-fix branch August 2, 2021 02:29
hangc0276 pushed a commit that referenced this pull request Aug 12, 2021
…out AppendBrokerTimestampMetadataInterceptor (#11014)

Fixes #11013

### Motivation

fix publish_time not set error when broker entry metadata enable without AppendBrokerTimestampMetadataInterceptor

### Modifications

1. add a new method named `getEntryTimestamp` which will return the `brokerEntryTimestamp` if `BrokerEntryMetadata` is enabled or otherwise return the `publishTime`.
2.  using this `entryTimestamp` for expiry checking.

(cherry picked from commit fe7cf67)
@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 12, 2021
@hangc0276 hangc0276 added this to the 2.9.0 milestone Aug 12, 2021
@hangc0276 hangc0276 added the type/bug The PR fixed a bug or issue reported a bug label Aug 12, 2021
sijie added a commit to streamnative/charts that referenced this pull request Aug 18, 2021
sijie added a commit to streamnative/charts that referenced this pull request Aug 18, 2021
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Oct 19, 2021
…out AppendBrokerTimestampMetadataInterceptor (apache#11014)

Fixes apache#11013

### Motivation

fix publish_time not set error when broker entry metadata enable without AppendBrokerTimestampMetadataInterceptor

### Modifications

1. add a new method named `getEntryTimestamp` which will return the `brokerEntryTimestamp` if `BrokerEntryMetadata` is enabled or otherwise return the `publishTime`.
2.  using this `entryTimestamp` for expiry checking.

(cherry picked from commit fe7cf67)
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…out AppendBrokerTimestampMetadataInterceptor (apache#11014)

Fixes apache#11013

### Motivation

fix publish_time not set error when broker entry metadata enable without AppendBrokerTimestampMetadataInterceptor

### Modifications

1. add a new method named `getEntryTimestamp` which will return the `brokerEntryTimestamp` if `BrokerEntryMetadata` is enabled or otherwise return the `publishTime`.
2.  using this `entryTimestamp` for expiry checking.
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
### Motivation

apache#11014 introduced `getEntryTimestamp` and removed `deserializeBrokerEntryMetaDataFirst`, but apache#11139 is still using `deserializeBrokerEntryMetaDataFirst`, and it breaks master branch.

### Modifications

use `entryTimestamp` for expiry checking in `internalGetMessageIdByTimestamp`

### Verifying this change

- [x] Make sure that the change passes the CI checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Field 'publish_time' is not set
3 participants