Skip to content

Conversation

@KannarFr
Copy link
Contributor

I started to implement #4547.

@KannarFr KannarFr force-pushed the tieredstorage-per-namespace branch from 1050eeb to b547171 Compare July 16, 2019 15:22
Copy link
Member

@sijie sijie left a 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?

@KannarFr KannarFr force-pushed the tieredstorage-per-namespace branch 2 times, most recently from c2f13c6 to 2696882 Compare July 17, 2019 11:52
Copy link
Contributor Author

@KannarFr KannarFr left a 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.

@KannarFr
Copy link
Contributor Author

I added the logic about retrieving offloadPolicies for each NamespaceName, and get a Map<NamespaceName, LedgerOffloadManager>, there are todo on FileSystem and TieredStorage way to override default tiered storage configuration when a custom for NamespaceName exists. Now I need to implment the logic to schedule and run the offload.

@KannarFr
Copy link
Contributor Author

@sijie The goal is to work with your reviews between each implement choices to ensure I'm doing well.

@sijie sijie requested a review from jiazhai July 18, 2019 16:46
@sijie sijie assigned sijie and KannarFr and unassigned sijie Jul 18, 2019
@sijie sijie added this to the 2.5.0 milestone Jul 18, 2019
@sijie
Copy link
Member

sijie commented Jul 18, 2019

@jiazhai can you please review this tiered storage related change?

@jiazhai
Copy link
Member

jiazhai commented Jul 29, 2019

It would be good to add some test for this change.

@KannarFr KannarFr force-pushed the tieredstorage-per-namespace branch from 258074f to 2e227e5 Compare July 30, 2019 08:45
Copy link
Contributor Author

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?

@KannarFr KannarFr force-pushed the tieredstorage-per-namespace branch from 2e227e5 to 7bbe81f Compare July 30, 2019 09:00
@KannarFr
Copy link
Contributor Author

KannarFr commented Aug 5, 2019

@sijie @jiazhai can you review before going further?

Copy link
Member

@sijie sijie left a 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?

@KannarFr KannarFr force-pushed the tieredstorage-per-namespace branch from 7bbe81f to 522c3de Compare August 12, 2019 12:17
Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiazhai maybe? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdbartholomew
Copy link
Contributor

I think this is a great feature. Is there anything I can do to help out?

@aahmed-se
Copy link
Contributor

@cdbartholomew rebase the pr on master and add more tests.

@sijie
Copy link
Member

sijie commented Nov 3, 2019

@aahmed-se I would suggest asking the original author first before taking over the PR.

@KannarFr
Copy link
Contributor Author

KannarFr commented Nov 3, 2019

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.

@cdbartholomew
Copy link
Contributor

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.

Copy link
Member

@sijie sijie left a 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.

@KannarFr
Copy link
Contributor Author

@cdbartholomew Hi, do you still want to work on it? :)

@cdbartholomew
Copy link
Contributor

@KannarFr sorry about taking so long to respond. I have been really busy and haven't been able to look at it. However, I still think this is a very useful change, and I think I have talked @zzzming into working on it. :-)

@KannarFr
Copy link
Contributor Author

KannarFr commented Jan 9, 2020

@cdbartholomew ok, I'm rebasing from master this PR.

@KannarFr KannarFr force-pushed the tieredstorage-per-namespace branch from 522c3de to 06e4584 Compare January 9, 2020 16:23
@jiazhai
Copy link
Member

jiazhai commented Jan 18, 2020

retest this please

@gaoran10
Copy link
Contributor

@KannarFr Hi, this is a good feature, I want to use it in my project, do you want finish it ?

@sijie
Copy link
Member

sijie commented Jan 18, 2020

@gaoran10 I guess @KannarFr is not actively working on this at this moment. If there is a need, feel free to continue the work based on this pull request.

@gaoran10
Copy link
Contributor

@sijie @KannarFr I'm glad to complete the feature base on this pull request.

@KannarFr
Copy link
Contributor Author

@gaoran10 Hi, go for it :).

@KannarFr
Copy link
Contributor Author

@gaoran10 any news? :)

@gaoran10
Copy link
Contributor

@KannarFr Hi, I finish this feature at #6183, have a look please.

@KannarFr
Copy link
Contributor Author

@gaoran10 Oh, you rewrite all commits? Anyway, thanks for the feature, can't wait to use it :p.

@codelipenghui
Copy link
Contributor

@KannarFr The #6183 merged, I would close this PR.

@KannarFr KannarFr deleted the tieredstorage-per-namespace branch February 10, 2020 12:57
@sijie
Copy link
Member

sijie commented Feb 13, 2020

Thanks @KannarFr for the contribution!

@codelipenghui
Copy link
Contributor

Since this feature is released in 2.5.1, so I added a release/2.5.1 tag for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants