Conversation
1050eeb to
b547171
Compare
sijie
left a comment
There was a problem hiding this comment.
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.
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.
@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
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.
But then I don't know which ledgerFactory should be used, not sure about what their job :/ can you explain them?
|
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. |
|
@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.