OVSDB Southbound utils
|
Vishal Thapar <vishal.thapar@...>
Hi OVSDB Committers,
Now that the Neutron auto release issue is resolved, I Wanted to take it to proper conclusion by making a decision on OVSDB Utils module. A recent change from Victor added dependency to ovsdb.library which had a knock on effect on dependents. The main reason for this was that OVSDB.Utils was not designed for external consumption but now is being used that way.
Considering this I’d like to propose making utils its own feature and have dependent projects use it if they want to use utils. I believe there is an issue with adding a new feature this late in cycle but considering recent impact on autorelease I think should be easy to geet waiver for it. Practically it is not a new feature, just a new wrapper to package code already being consumed downstream projects.
Thoughts?
Regards, Vishal.
|
|
|
|
Sam Hague
I like the idea to make a feature. It breaks it out the way we want so it can be consumed.
On Tue, Aug 8, 2017 at 12:12 AM, Vishal Thapar <vishal.thapar@...> wrote:
|
|
|
|
Victor Pickard <vpickard@...>
Agree with the idea to make it a feature for consumption. Vic
On Tue, Aug 8, 2017 at 6:48 AM, Sam Hague <shague@...> wrote:
|
|
|
|
Anil Vishnoi
Personally i like the idea of exposing concrete util API's as it would help project to avoid writing repeating code. But before we jump on it, just want to put all the facts about it for discussion: Currently utils classes expose following number of public methods ( HwvtepSouthboundUtils.java --> 11 Public methods(Most of them are just returning builder objects, IIDs) MdsalUtils.java --> 6 Public methods(Blocked Wrapper around MDSAL operations) MdsalUtilsAsync.java --> 7 Public methods(Async Wrapper around MDSAL operations) NotifyingDataChangeListener.java -> 16 Public methods ( Methods around data change notifications) Skipping ovsdb-it-utils ServiceHelper.java --> 3 public methods (OSGi service related methods) SouthboundUtils.java --> 86 public methods (Mostly ovsdb southbound plugin related util methods) YangUtils.java --> 2 public methods (yang keyvalue related methods). So overall we will be exposing around 130 public methods to all the downstream consumers. If we expose them in the current form, they all will be considered as a public API's. That will have following impact on the way we currently add methods to it (1) We need a really exception waiver to expose 130 public API's just near the RC0 timeline. (2) Any future change in any API's will have to go through a deprecation process (Mark api for 6 month and then remove it). (3) To modify any of the existing APIs (syntactically,semantically), even for a minor change, you will have to make sure that all the project using these API's are notified and they have a gerrit patch ready for the change. Additionally after M4, i believe you need to get API waiver. Given the current timeline, i am not really comfortable in just exposing these public methods as an APIs for the following reasons: (1) These API's are not clearly defined (no proper documentation ) (2) Some of the public methods don't qualify for API (3) We would be directly exposing the implementation of the Util class, so user can even call the close() method as well. (4) There is no proper structure in the code (no interfaces defining the API's that we want expose, separation of api/impl etc). (5) No unit test, so no way to catch regression. I think if we really want to expose these methods as a API, we should address all the above issues before we expose these APIs. For a meantime, i think we can workaround the dependency issue that is introduced by victor's patch by refactoring the util method to remove the library dependency. Here is the first shot at refactoring the method public static boolean compareDbVersionToMinVersion(final String dbVersion, final String minVersion) { On Tue, Aug 8, 2017 at 6:11 AM, Victor Pickard <vpickard@...> wrote:
Thanks Anil
|
|
|
|
Vishal Thapar <vishal.thapar@...>
+1 to both.
Utils has become a mix of public+internal APIs which is a problem in itself. Note that currently it is not exposed as official API, but still being consumed like one. Modifying the API is the simplest change that breaks the dpendency but doesn’t need changes to downstream projects.
Let me know if you’re raising a patch for this or want me to.
Regards, Vishal.
From: ovsdb-dev-bounces@... [mailto:ovsdb-dev-bounces@...]
On Behalf Of Anil Vishnoi
Personally i like the idea of exposing concrete util API's as it would help project to avoid writing repeating code.
But before we jump on it, just want to put all the facts about it for discussion:
Currently utils classes expose following number of public methods ( HwvtepSouthboundUtils.java --> 11 Public methods(Most of them are just returning builder objects, IIDs) MdsalUtils.java --> 6 Public methods(Blocked Wrapper around MDSAL operations) MdsalUtilsAsync.java --> 7 Public methods(Async Wrapper around MDSAL operations) NotifyingDataChangeListener.java -> 16 Public methods ( Methods around data change notifications) Skipping ovsdb-it-utils ServiceHelper.java --> 3 public methods (OSGi service related methods) SouthboundUtils.java --> 86 public methods (Mostly ovsdb southbound plugin related util methods) YangUtils.java --> 2 public methods (yang keyvalue related methods).
So overall we will be exposing around 130 public methods to all the downstream consumers. If we expose them in the current form, they all will be considered as a public API's. That will have following impact on the way we currently add methods to it (1) We need a really exception waiver to expose 130 public API's just near the RC0 timeline. (2) Any future change in any API's will have to go through a deprecation process (Mark api for 6 month and then remove it). (3) To modify any of the existing APIs (syntactically,semantically), even for a minor change, you will have to make sure that all the project using these API's are notified and they have a gerrit patch ready for the change. Additionally after M4, i believe you need to get API waiver.
Given the current timeline, i am not really comfortable in just exposing these public methods as an APIs for the following reasons: (1) These API's are not clearly defined (no proper documentation ) (2) Some of the public methods don't qualify for API (3) We would be directly exposing the implementation of the Util class, so user can even call the close() method as well. (4) There is no proper structure in the code (no interfaces defining the API's that we want expose, separation of api/impl etc). (5) No unit test, so no way to catch regression.
I think if we really want to expose these methods as a API, we should address all the above issues before we expose these APIs.
For a meantime, i think we can workaround the dependency issue that is introduced by victor's patch by refactoring the util method to remove the library dependency. Here is the first shot at refactoring the method
On Tue, Aug 8, 2017 at 6:11 AM, Victor Pickard <vpickard@...> wrote:
-- Thanks Anil
|
|
|
|
Victor Pickard <vpickard@...>
Anil, Nice work breaking this out, gives us all a better understanding of what is involved. I agree with your proposal, break the dependency by fully implementing compareDbVersionToMinVersion(). Thanks
On Sun, Aug 13, 2017 at 11:47 PM, Vishal Thapar <vishal.thapar@...> wrote:
|
|
|
|
Sam Hague
Anil,
toggle quoted messageShow quoted text
good plan to port over the version compare to the utils. Sam
On Mon, Aug 14, 2017 at 9:28 AM, Victor Pickard <vpickard@...> wrote:
Anil,
|
|
|
|
matthias.backhausen@ts.fujitsu.com <matthias.backhausen@...>
please can you remove me from this list. I several times tried it but without success. Many thanks! Matthias
Mit TouchDown von meinem Android-Telefon gesendet (www.symantec.com)
-----Original Message-----
From: Sam Hague [shague@...] Received: Montag, 14 Aug. 2017, 16:23 To: Victor Pickard [vpickard@...] CC: Stephen Kitt [skitt@...]; {ovsdb-dev@...} [ovsdb-dev@...] Subject: Re: [ovsdb-dev] OVSDB Southbound utils Anil,
good plan to port over the version compare to the utils. Sam On Mon, Aug 14, 2017 at 9:28 AM, Victor Pickard <vpickard@...> wrote: > Anil, > Nice work breaking this out, gives us all a better understanding of what is > involved. > > I agree with your proposal, break the dependency by fully implementing > compareDbVersionToMinVersion(). > > Thanks > > On Sun, Aug 13, 2017 at 11:47 PM, Vishal Thapar <vishal.thapar@...> > wrote: >> >> +1 to both. >> >> >> >> Utils has become a mix of public+internal APIs which is a problem in >> itself. Note that currently it is not exposed as official API, but still >> being consumed like one. Modifying the API is the simplest change that >> breaks the dpendency but doesn’t need changes to downstream projects. >> >> >> >> Let me know if you’re raising a patch for this or want me to. >> >> >> >> Regards, >> >> Vishal. >> >> >> >> From: ovsdb-dev-bounces@... >> [mailto:ovsdb-dev-bounces@...] On Behalf Of Anil Vishnoi >> Sent: 12 August 2017 13:31 >> To: Victor Pickard <vpickard@...> >> Cc: Stephen Kitt <skitt@...>; <ovsdb-dev@...> >> <ovsdb-dev@...> >> Subject: Re: [ovsdb-dev] OVSDB Southbound utils >> >> >> >> Personally i like the idea of exposing concrete util API's as it would >> help project to avoid writing repeating code. >> >> >> >> But before we jump on it, just want to put all the facts about it for >> discussion: >> >> >> >> Currently utils classes expose following number of public methods ( >> >> HwvtepSouthboundUtils.java --> 11 Public methods(Most of them are just >> returning builder objects, IIDs) >> >> MdsalUtils.java --> 6 Public methods(Blocked Wrapper around MDSAL >> operations) >> >> MdsalUtilsAsync.java --> 7 Public methods(Async Wrapper around MDSAL >> operations) >> >> NotifyingDataChangeListener.java -> 16 Public methods ( Methods around >> data change notifications) >> >> Skipping ovsdb-it-utils >> >> ServiceHelper.java --> 3 public methods (OSGi service related methods) >> >> SouthboundUtils.java --> 86 public methods (Mostly ovsdb southbound plugin >> related util methods) >> >> YangUtils.java --> 2 public methods (yang keyvalue related methods). >> >> >> >> So overall we will be exposing around 130 public methods to all the >> downstream consumers. If we expose them in the current form, they all will >> be considered as a public API's. That will have following impact on the way >> we currently add methods to it >> >> (1) We need a really exception waiver to expose 130 public API's just near >> the RC0 timeline. >> >> (2) Any future change in any API's will have to go through a deprecation >> process (Mark api for 6 month and then remove it). >> >> (3) To modify any of the existing APIs (syntactically,semantically), even >> for a minor change, you will have to make sure that all the project using >> these API's are notified and they have a gerrit patch ready for the change. >> Additionally after M4, i believe you need to get API waiver. >> >> >> >> Given the current timeline, i am not really comfortable in just exposing >> these public methods as an APIs for the following reasons: >> >> (1) These API's are not clearly defined (no proper documentation ) >> >> (2) Some of the public methods don't qualify for API >> >> (3) We would be directly exposing the implementation of the Util class, so >> user can even call the close() method as well. >> >> (4) There is no proper structure in the code (no interfaces defining the >> API's that we want expose, separation of api/impl etc). >> >> (5) No unit test, so no way to catch regression. >> >> >> >> I think if we really want to expose these methods as a API, we should >> address all the above issues before we expose these APIs. >> >> >> >> For a meantime, i think we can workaround the dependency issue that is >> introduced by victor's patch by refactoring the util method to remove the >> library dependency. Here is the first shot at refactoring the method >> >> >> >> public static boolean compareDbVersionToMinVersion(final String >> dbVersion, final String minVersion) { >> final String FORMAT = "(\\d+)\\.(\\d+)\\.(\\d+)"; >> final Pattern PATTERN = Pattern.compile(FORMAT); >> final Matcher dbVersionMatcher = PATTERN.matcher(dbVersion); >> final Matcher minVersionMatcher = PATTERN.matcher(minVersion); >> if (dbVersion != null && !dbVersion.isEmpty() && minVersion != >> null >> && !minVersion.isEmpty()) { >> if >> ((Integer.valueOf(dbVersionMatcher.group(1)).equals(Integer.valueOf(minVersionMatcher.group(1))) >> && >> >> Integer.valueOf(dbVersionMatcher.group(2)).equals(Integer.valueOf(minVersionMatcher.group(2))) >> && >> >> Integer.valueOf(dbVersionMatcher.group(3)).equals(Integer.valueOf(minVersionMatcher.group(3)))) >> || >> Integer.valueOf(dbVersionMatcher.group(1)).intValue() >> > Integer.valueOf(minVersionMatcher.group(1)).intValue() || >> Integer.valueOf(dbVersionMatcher.group(2)).intValue() >> >= Integer.valueOf(minVersionMatcher.group(2)).intValue() || >> Integer.valueOf(dbVersionMatcher.group(3)).intValue() >> >= Integer.valueOf(minVersionMatcher.group(3)).intValue()) { >> return true; >> } >> } >> return false; >> } >> >> >> >> >> >> >> >> On Tue, Aug 8, 2017 at 6:11 AM, Victor Pickard <vpickard@...> >> wrote: >> >> Agree with the idea to make it a feature for consumption. >> >> >> >> Vic >> >> >> >> >> >> On Tue, Aug 8, 2017 at 6:48 AM, Sam Hague <shague@...> wrote: >> >> I like the idea to make a feature. It breaks it out the way we want so it >> can be consumed. >> >> >> >> On Tue, Aug 8, 2017 at 12:12 AM, Vishal Thapar >> <vishal.thapar@...> wrote: >> >> Hi OVSDB Committers, >> >> >> >> Now that the Neutron auto release issue is resolved, I Wanted to take it >> to proper conclusion by making a decision on OVSDB Utils module. A recent >> change from Victor added dependency to ovsdb.library which had a knock on >> effect on dependents. The main reason for this was that OVSDB.Utils was not >> designed for external consumption but now is being used that way. >> >> >> >> Considering this I’d like to propose making utils its own feature and have >> dependent projects use it if they want to use utils. I believe there is an >> issue with adding a new feature this late in cycle but considering recent >> impact on autorelease I think should be easy to geet waiver for it. >> Practically it is not a new feature, just a new wrapper to package code >> already being consumed downstream projects. >> >> >> >> Thoughts? >> >> >> >> Regards, >> >> Vishal. >> >> >> >> >> >> _______________________________________________ >> ovsdb-dev mailing list >> ovsdb-dev@... >> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev >> >> >> >> >> _______________________________________________ >> ovsdb-dev mailing list >> ovsdb-dev@... >> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev >> >> >> >> >> >> -- >> >> Thanks >> >> Anil > > > > _______________________________________________ > ovsdb-dev mailing list > ovsdb-dev@... > https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev > _______________________________________________ ovsdb-dev mailing list ovsdb-dev@... https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
|
|
|
|
Anil Vishnoi
Great!, @Vishal, feel free to push the patch. I will review and merge it.
On Mon, Aug 14, 2017 at 7:22 AM, Sam Hague <shague@...> wrote: Anil, --
Thanks Anil
|
|
|
|
Victor Pickard <vpickard@...>
Vishal, Have you started on this patch? If not, I will push one.
On Mon, Aug 14, 2017 at 11:57 PM, Anil Vishnoi <vishnoianil@...> wrote:
|
|
|
|
Vishal Thapar <vishal.thapar@...>
I’ve pushed the patch: https://git.opendaylight.org/gerrit/61905
My dev setup is hosed post version bump so was unable to build locally. Pushed the patch but haven’t tested yet. Once I refresh my local repo, will test it out.
Regards, Vishal.
From: ovsdb-dev-bounces@... [mailto:ovsdb-dev-bounces@...]
On Behalf Of Victor Pickard
Vishal, Have you started on this patch? If not, I will push one.
On Mon, Aug 14, 2017 at 11:57 PM, Anil Vishnoi <vishnoianil@...> wrote:
|
|
|