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.

Sign up for GitHub

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

Jump to bottom

[offload] fix FileSystemManagedLedgerOffloader can not cleanup outdated ledger #12309

Merged
merged 1 commit into from
Oct 13, 2021
Merged

[offload] fix FileSystemManagedLedgerOffloader can not cleanup outdated ledger #12309

merged 1 commit into from
Oct 13, 2021

Conversation

frankxieke
Copy link
Contributor

@frankxieke frankxieke commented Oct 9, 2021 •
edited
Loading

Motivation

  • When using FileSystem Offloader, and you set the ledger retention policy, after the offloaded ledger is outdated, however the data can not be deleted from file system. This is because the datafile path is wrong.

  • For example, in fact the datafile path is "file:///Users/pulsar_nfs/public/default/persistent/test_pulsar_delta/449-775f0961-9719-4658-8357-6b0edbdef7a3", but is mistaken formatted as "file:///Users/pulsar_nfs/null/449-775f0961-9719-4658-8357-6b0edbdef7a3".

  • The reason is when format the data path, "ManagedLedgerName" property in "offloadDriverMetadata" is missing.

Modifications

  • Before format the data path, add "ManagedLedgerName" property in "offloadDriverMetadata" map.

Verifying this change

  • Make sure that the change passes the CI checks.

I will add a new unit test to cover this.

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

  • 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)

Documentation

Need to update docs?

  • no-need-doc

    fix bug, not affect user using the offload.

@codelipenghui codelipenghui added this to the 2.10.0 milestone Oct 9, 2021
congbobo184
congbobo184 approved these changes Oct 9, 2021
View reviewed changes
Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

LGTM! Good catch!

hangc0276
hangc0276 approved these changes Oct 9, 2021
View reviewed changes
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.

Good Job!

hangc0276
hangc0276 requested changes Oct 9, 2021
View reviewed changes
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.

if the offloadDriverMetadata generated by Collections.emptyMap(), and then call put operation, it will throw the following exception:

java.lang.UnsupportedOperationException
	at java.util.AbstractMap.put(AbstractMap.java:209)

eolivelli
eolivelli reviewed Oct 10, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

I wonder why this is not caught by existing unit tests.
It would be better to add a test that cover this code.

@frankxieke frankxieke force-pushed the fix_filesystem_offloader_cannot_cleanup branch from 4f009ed to 5298e84 Compare October 12, 2021 02:05
@frankxieke frankxieke requested a review from hangc0276 October 12, 2021 02:08
@frankxieke frankxieke force-pushed the fix_filesystem_offloader_cannot_cleanup branch from 5298e84 to c08ee9a Compare October 12, 2021 09:02
@frankxieke frankxieke force-pushed the fix_filesystem_offloader_cannot_cleanup branch from c08ee9a to 55f4708 Compare October 12, 2021 09:07
@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Oct 13, 2021
@hangc0276 hangc0276 merged commit f283030 into apache:master Oct 13, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 14, 2021
Merge branch 'up/master' into website/branch-2.7.2-chapter-2
8b9e28c 
* up/master: (26 commits)
  [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128)
  [website][upgrade]feat: docs migration - Development (apache#12320)
  Update delete inactive topic configuration documentation (apache#12350)
  [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056)
  Added Debezium Source for MS SQL Server (apache#12256)
  Fix: flaky oracle tests (apache#12306)
  [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341)
  [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
  feat(cli): add restart command to pulsar-daemon (apache#12279)
  [client-tools] Remove redundant initial value (apache#12296)
  Make AuthenticationTokenTest to run on windows (apache#12329)
  [offload] fix FileSystemManagedLedgerOffloader can not cleanup outdated ledger data (apache#12309)
  [Doc]--Update contents for Pulsar adaptor for Apache Spark (apache#12338)
  [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode (apache#12066)
  [website][upgrade]feat: docs migration - Cookbooks (apache#12319)
  [testclient] Make --payload-file take effect in PerformanceClient (apache#12187)
  [website][upgrade]feat: docs migration - adaptor (apache#12318)
  [pulsar-client] Add partition-change api for producer/consumer interceptors (apache#12287)
  [Transaction]Fix lowWaterMark of TopicTransactionBuffer (apache#12312)
  [pulsar-admin] New option takes precedence over deprecated option (apache#12260)
  ...

# Conflicts:
#	site2/website-next/docusaurus.config.js
#	site2/website-next/versions.json
hangc0276 pushed a commit that referenced this pull request Oct 17, 2021
@frankxieke @hangc0276
[offload] fix FileSystemManagedLedgerOffloader can not cleanup outdat…
660ee85 
…ed ledger data (#12309)

### Motivation
When using FileSystem Offloader, and you set the ledger retention policy, after the offloaded ledger is outdated, however the data can not be deleted from file system. This is because the datafile path is wrong.

For example, in fact the datafile path is "file:///Users/pulsar_nfs/public/default/persistent/test_pulsar_delta/449-775f0961-9719-4658-8357-6b0edbdef7a3", but is mistaken formatted as "file:///Users/pulsar_nfs/null/449-775f0961-9719-4658-8357-6b0edbdef7a3".

The reason is when format the data path, "ManagedLedgerName" property in "offloadDriverMetadata" is missing.

### Modifications
Before format the data path, add "ManagedLedgerName" property in "offloadDriverMetadata" map.

(cherry picked from commit f283030)
@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 17, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@frankxieke @ciaocloud
[offload] fix FileSystemManagedLedgerOffloader can not cleanup outdat…
8107b75 
…ed ledger data (apache#12309)

### Motivation
When using FileSystem Offloader, and you set the ledger retention policy, after the offloaded ledger is outdated, however the data can not be deleted from file system. This is because the datafile path is wrong.

For example, in fact the datafile path is "file:///Users/pulsar_nfs/public/default/persistent/test_pulsar_delta/449-775f0961-9719-4658-8357-6b0edbdef7a3", but is mistaken formatted as "file:///Users/pulsar_nfs/null/449-775f0961-9719-4658-8357-6b0edbdef7a3".

The reason is when format the data path, "ManagedLedgerName" property in "offloadDriverMetadata" is missing.

### Modifications
Before format the data path, add "ManagedLedgerName" property in "offloadDriverMetadata" map.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli left review comments

@congbobo184 congbobo184 congbobo184 approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@hangc0276 hangc0276 hangc0276 approved these changes

@315157973 315157973 Awaiting requested review from 315157973

Assignees

@frankxieke frankxieke

Labels
area/tieredstorage cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
7 participants
@frankxieke @hangc0276 @eolivelli @codelipenghui @gaoran10 @congbobo184 @Anonymitaet