-
Notifications
You must be signed in to change notification settings - Fork 3.7k
offload bucket per namespace #4739
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
Conversation
1050eeb to
b547171
Compare
sijie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this PR only introduces the offload policies but it doesn't implement the logic to let the offloader using the offload polices. Is that done intentionally?
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/tieredStorage/OffloadType.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPolicies.java
Outdated
Show resolved
Hide resolved
c2f13c6 to
2696882
Compare
KannarFr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed OffloadType to String. And fixed other points.
|
I added the logic about retrieving offloadPolicies for each NamespaceName, and get a |
|
@sijie The goal is to work with your reviews between each implement choices to ensure I'm doing well. |
|
@jiazhai can you please review this tiered storage related change? |
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/LedgerOffloaderFactory.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
|
It would be good to add some test for this change. |
258074f to
2e227e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jiazhai I removed it because of the new lazy load feature of offload policies even for non defined offloaders. WDYT about removing this?
2e227e5 to
7bbe81f
Compare
sijie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KannarFr overall looks good. However I didn't see how the offload policies used by actual offloader. do I miss anything?
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
Outdated
Show resolved
Hide resolved
7bbe81f to
522c3de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sijie If I'm correct, it's done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then I don't know which ledgerFactory should be used, not sure about what their job :/ can you explain them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jiazhai maybe? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I think this is a great feature. Is there anything I can do to help out? |
|
@cdbartholomew rebase the pr on master and add more tests. |
|
@aahmed-se I would suggest asking the original author first before taking over the PR. |
|
Hey, was waiting for feedback about the way I already done some work. I will be pleased to have your help. Sure first thing is to rebase from master. |
Ok, I'll rebase from master and take a look at adding some tests. |
sijie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KannarFr the general direction of this change looks good to me.
|
@cdbartholomew Hi, do you still want to work on it? :) |
|
@cdbartholomew ok, I'm rebasing from master this PR. |
…offloadPolicies parameter
522c3de to
06e4584
Compare
|
retest this please |
|
@KannarFr Hi, this is a good feature, I want to use it in my project, do you want finish it ? |
|
@gaoran10 Hi, go for it :). |
|
@gaoran10 any news? :) |
|
@gaoran10 Oh, you rewrite all commits? Anyway, thanks for the feature, can't wait to use it :p. |
|
Thanks @KannarFr for the contribution! |
|
Since this feature is released in 2.5.1, so I added a |
I started to implement #4547.