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

[Issue 13217][pulsar-package-management] Fix the filesystem storage failure #13218

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

murong00
Copy link
Contributor

@murong00 murong00 commented Dec 9, 2021

Motivation

FIxes #13217.

Modifications

Make packagePath and metadataPath at the same level.

Documentation

  • no-need-doc

@murong00 murong00 requested a review from zymap December 9, 2021 14:07
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 9, 2021
@merlimat merlimat added this to the 2.10.0 milestone Dec 9, 2021
@@ -248,7 +248,7 @@ private String metadataPath(PackageName packageName) {
}

private String packagePath(PackageName packageName) {
return packageName.toRestPath();
return packageName.toRestPath() + "/pack";
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest changing the /pack to /data

Copy link
Member

Choose a reason for hiding this comment

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

BTW, could you please add a test to cover this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, we should delete the packageName.toRestPath() path at the same time otherwise list-versions command will print this parent path (i.e. version path like v1.0).

Copy link
Member

@zymap zymap 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 we need to add a test to cover this case

@murong00
Copy link
Contributor Author

I think we need to add a test to cover this case

Sure, I'm working on this.

@murong00
Copy link
Contributor Author

@zymap I have addressed above comments, please help to take a review again.

@murong00
Copy link
Contributor Author

/pulsarbot run-failure-checks

@murong00
Copy link
Contributor Author

ping @zymap

@murong00 murong00 merged commit 025e082 into apache:master Dec 13, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
…ailure (apache#13218)

* Fix the filesystem storage failure

* Add a integration test to cover this change
zymap added a commit to zymap/pulsar that referenced this pull request Apr 28, 2022
---

Fixes apache#15362

*Motivation*

The PR apache#13218 supports saving
package data into filesystem. But it introduces an regression for the
old versions.
For example, we have a package function://public/default/package@v0.1,
it will save the meta to the path function/public/default/package/v0.1/meta,
and save the data to the path function/public/default/package/v0.1.
By default, we are using distributed log as the package storage, and
it supports saving data at a directory.
But some storage like filesystem don't have the similar ability, it
needs another path for saving data.
This api provides the ability to support saving the data in another place.
If you specify the data path as `/data`, the package will saved into
function/public/default/package/v0.1/data.

*Modifications*

- make the data path configurable in the storage implementation.
zymap added a commit that referenced this pull request May 6, 2022
…ion (#15367)

---

Fixes #15362

*Motivation*

The PR #13218 supports saving
package data into filesystem. But it introduces a regression for the
old versions.
For example, we have a package function://public/default/package@v0.1,
it will save the meta to the path function/public/default/package/v0.1/meta,
and save the data to the path function/public/default/package/v0.1.
By default, we are using distributed log as the package storage, and
it supports saving data in a directory.
But some storage like filesystem doesn't have the similar ability, it
needs another path for saving data.
This API provides the ability to support saving the data in another place.
If you specify the data path as `/data`, the package will be saved into
function/public/default/package/v0.1/data.

*Modifications*

- make the data path configurable in the storage implementation.
codelipenghui pushed a commit that referenced this pull request May 20, 2022
…ion (#15367)

---

Fixes #15362

*Motivation*

The PR #13218 supports saving
package data into filesystem. But it introduces a regression for the
old versions.
For example, we have a package function://public/default/package@v0.1,
it will save the meta to the path function/public/default/package/v0.1/meta,
and save the data to the path function/public/default/package/v0.1.
By default, we are using distributed log as the package storage, and
it supports saving data in a directory.
But some storage like filesystem doesn't have the similar ability, it
needs another path for saving data.
This API provides the ability to support saving the data in another place.
If you specify the data path as `/data`, the package will be saved into
function/public/default/package/v0.1/data.

*Modifications*

- make the data path configurable in the storage implementation.

(cherry picked from commit aa9aa18)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 23, 2022
…ion (apache#15367)

---

Fixes apache#15362

*Motivation*

The PR apache#13218 supports saving
package data into filesystem. But it introduces a regression for the
old versions.
For example, we have a package function://public/default/package@v0.1,
it will save the meta to the path function/public/default/package/v0.1/meta,
and save the data to the path function/public/default/package/v0.1.
By default, we are using distributed log as the package storage, and
it supports saving data in a directory.
But some storage like filesystem doesn't have the similar ability, it
needs another path for saving data.
This API provides the ability to support saving the data in another place.
If you specify the data path as `/data`, the package will be saved into
function/public/default/package/v0.1/data.

*Modifications*

- make the data path configurable in the storage implementation.

(cherry picked from commit aa9aa18)
(cherry picked from commit 46d6a7f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package management filesystem storage doesn't work
3 participants