Skip to content

Conversation

@prashantpogde
Copy link
Contributor

What changes were proposed in this pull request?

Implement post-finalize SCM logic to allow nodes of only new version to participate in pipelines.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4179

How was this patch tested?

build UT. I will fix failures in CI.

Copy link
Contributor

@avijayanhwx avijayanhwx left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @prashantpogde. A few comments inline.

@fapifta
Copy link
Contributor

fapifta commented Nov 25, 2020

Hi Prashant, thank you for working on this one, great work so far!

I am thinking about the refactor of the UpgradeFinalizer.
In the BasicFinalizer, there is a finalizeFeature, that is called from the SCMUpgradeFinalizer, while the OMUpgradeFinalizer has its own finalizeFeature method.

What if we generalize this one to be the one in BasicUpgradeFinalizer along with the worker, or why don't we?
Roughly this is what I would try:

  • BasicUpgradeFinalizer defines the Worker, and we encapsulate the component into a wrapper behind an interface and use that in the worker.
  • the component's wrapper should have the two hooks, preFinalizeUpgrade and postFinalizeUpgrade
  • if we add a getStorage method we can avoid the problem that comes from the different Storage getters as well
  • we can add any other required stuff into the wrapper, and cut off the upgrade finalization logic from the component's implementation without cutting the UpgradeActions, as there we can still provide the component from the wrapper.

So at the end of the day we may get to one generic UpgradeFinalizer, that uses a let's say UpgradingComponent interface for which the implementations may be OzoneManagerComponent, and StorageContainerManagerComponent.

I started to think about this because of the similarities in the two concrete UpgradeFinalizer, and the two pretty much different approach on how we handled the Optional UpgradeAction :)

@prashantpogde
Copy link
Contributor Author

I am thinking about the refactor of the UpgradeFinalizer.
In the BasicFinalizer, there is a finalizeFeature, that is called from the SCMUpgradeFinalizer, while the OMUpgradeFinalizer has its own finalizeFeature method.
....
....
I started to think about this because of the similarities in the two concrete UpgradeFinalizer, and the two pretty much different approach on how we handled the Optional UpgradeAction :

Earlier I was keeping worker also in the Basic UpgradeFinalizer as you suggested but it seemed Om finalization didnt need pre/post hook. So left it this way. We can also discuss this in a call.

@fapifta
Copy link
Contributor

fapifta commented Nov 26, 2020

Even though the wrapper for OM does nothing in the pre/post hook, it might worth to write the code once and just give an empty implementation for the hooks. Sure let's discuss this one during one of our next calls.

@prashantpogde
Copy link
Contributor Author

TestOmBlockVersioning failure is unrelated. Its passing on my local setup.

@fapifta
Copy link
Contributor

fapifta commented Dec 3, 2020

Thanks @prashantpogde for working on this one, and addressing the review comments.
The changes look good to me, as we discussed the proposed refactoring is something we might get back to but we would defer it for now.

+1 from my side.

Copy link
Contributor

@avijayanhwx avijayanhwx left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments @prashantpogde.

@avijayanhwx
Copy link
Contributor

CI failure is on OM side, while the changes are on HDDS side. Unable to repro this on my local setup.

@avijayanhwx avijayanhwx merged commit c3e3b35 into apache:HDDS-3698-upgrade Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants