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:

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.



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:
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@....org
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev



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) {
        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@...rg
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev



_______________________________________________
ovsdb-dev mailing list
ovsdb-dev@....org
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev




--
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
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


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:

+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@lists.opendaylight.org [mailto:ovsdb-dev-bounces@lists.opendaylight.org] On Behalf Of Anil Vishnoi
Sent: 12 August 2017 13:31
To: Victor Pickard <vpickard@...>
Cc: Stephen Kitt <skitt@...>; <ovsdb-dev@....org> <ovsdb-dev@....org>
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@....org
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev

 


_______________________________________________
ovsdb-dev mailing list
ovsdb-dev@....org
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev



 

--

Thanks

Anil



Sam Hague
 

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


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,

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@lists.opendaylight.org
>> [mailto:ovsdb-dev-bounces@lists.opendaylight.org] On Behalf Of Anil Vishnoi
>> Sent: 12 August 2017 13:31
>> To: Victor Pickard <vpickard@...>
>> Cc: Stephen Kitt <skitt@...>; <ovsdb-dev@....org>
>> <ovsdb-dev@....org>
>> 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@....org
>> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
>>
>>
>>
>>
>> _______________________________________________
>> ovsdb-dev mailing list
>> ovsdb-dev@....org
>> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
>>
>>
>>
>>
>>
>> --
>>
>> Thanks
>>
>> Anil
>
>
>
> _______________________________________________
> ovsdb-dev mailing list
> ovsdb-dev@....org
> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
>
_______________________________________________
ovsdb-dev mailing list
ovsdb-dev@....org
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev



--
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:
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,

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@...ylight.org
>> [mailto:ovsdb-dev-bounces@lists.opendaylight.org] On Behalf Of Anil Vishnoi
>> Sent: 12 August 2017 13:31
>> To: Victor Pickard <vpickard@...>
>> Cc: Stephen Kitt <skitt@...>; <ovsdb-dev@....org>
>> <ovsdb-dev@....org>
>> 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@...rg
>> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
>>
>>
>>
>>
>> _______________________________________________
>> ovsdb-dev mailing list
>> ovsdb-dev@...rg
>> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
>>
>>
>>
>>
>>
>> --
>>
>> Thanks
>>
>> Anil
>
>
>
> _______________________________________________
> ovsdb-dev mailing list
> ovsdb-dev@...rg
> https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev
>
_______________________________________________
ovsdb-dev mailing list
ovsdb-dev@...rg
https://lists.opendaylight.org/mailman/listinfo/ovsdb-dev



--
Thanks
Anil


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
Sent: 16 August 2017 19:08
To: Anil Vishnoi <vishnoianil@...>
Cc: Stephen Kitt <skitt@...>; <ovsdb-dev@...> <ovsdb-dev@...>
Subject: Re: [ovsdb-dev] OVSDB Southbound utils

 

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:

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,

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



 

--

Thanks

Anil