[cygnus-ngsi] NGSI-LD support

[anmunoz]

Hello,

I am Andrés from Technical University of Madrid. From UPM have been working with FIWARE Foundation for trying to define a first development for providing NGSI-LD support to Cygnus. We identify that we can provide this new feature by adding new classes for NGSI-LD that works well with the current version of Cygnus. We have been working in the source part for allowing to build NGSI-LD objects for NGSI-LD notifications, using a Modified versión of NGSIRest Handler, and the first sink provided for this implementation is the NGSILDPostgresqlSink. We have also available the documentation about this new feature in our repository, https://github.com/anmunoz/fiware-cygnus/blob/master/doc/cygnus-ngsi/flume_extensions_catalogue/ngsi_ld_rest_handler.md and https://github.com/anmunoz/fiware-cygnus/blob/master/doc/cygnus-ngsi/flume_extensions_catalogue/ngsi_ld_postgresql_sink.md

We are preparing a pull request with the changes needed to add this new feature, Are you agree with this idea?. If you have any comment or need more information about it, please let me know.

[mrutid]

Hi Andrés nice to hear from you.

We have make a quick review of your fork. Let me make some initial comments: we are concerned about how the new functionality is packaged. It's weird to have two "families" of sinks within the same package (the NGSI and the NGSILD sinks) since they are not interoperable. Till this moment we can choose whichever Sink we need from the package. This is the desired behavior.

On the other hand it is noticeable that there is no relationship between LD sinks an V2 sinks... I suppose there should be some kind os delegation or heritage due to common functionality (maybe there is no common functionality, it would be nice to elaborate this point).

Last comment, please take a look to the Cygnus-twitter package (at root level), when we change the kind of event (new kind of agent/source) we like to diferentiate the cygnus bundle. Probably this is the case for NGSILD y this would solve the issues above also.

We will wait for the PR (please don't forget to include licensing and IPR headers), thank you in advance for your contribution.

[fgalan]

Please find some more precision in some of the items that @mrutid mentions.

On the other hand it is noticeable that there is no relationship between LD sinks an V2 sinks... I suppose there should be some kind os delegation or heritage due to common functionality (maybe there is no common functionality, it would be nice to elaborate this point).

As far as I have analyzed in your code, NGSILDPostgreSQLSink extends NGSILDSink and NGSILDSink in secuence extends CygnusSink (which I understand belong to the Cygnus upstream code). However, why don't making NGSILDPostgreSQLSink to extend NGSIPostgreSink? I understand the relationship is closer.

We will wait for the PR (please don't forget to include licensing and IPR headers), thank you in advance for your contribution.

In particular, header should be like this in every new file:

/**
 * Copyright 2020 Telefonica Investigación y Desarrollo, S.A.U
 *
 * This file is part of fiware-cygnus (FIWARE project).
 *
 * fiware-cygnus is free software: you can redistribute it and/or modify it under the terms of the GNU Affero
 * General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your
 * option) any later version.
 * fiware-cygnus is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the
 * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License
 * for more details.
 *
 * You should have received a copy of the GNU Affero General Public License along with fiware-cygnus. If not, see
 * http://www.gnu.org/licenses/.
 *
 * For those usages not covered by the GNU Affero General Public License please contact with iot_support at tid dot es
 */

Thanks!

[anmunoz]

Hello @mrutid and @fgalan ,
Thanks for the feedback. I want to elaborate a little more in some of the points that you mention to have a better perspective of how we can contribute before creating the PR.

First of all I wanted to mention that the approach that we took was try to not touch the previous code, in this case all of the developments made for NGSIv2. That is the main reason why we decide not to work over previous classes Like NGSIEvent, NGSIBatch, and soo on.

On one hand, I agree that a good way to incorporate this new development is in a separate package. However is not clear for me what will be the most accurate approach. Since changing the packaging name or heritage may generate compatibility issues with the past versions of cygnus and the new created with this new functionality. For example, in cygnus-ngsi we can introduce in the sinks package (com.telefonica.iot.cygnus.sinks), ngsiv2 and move inside them all of the sinks that work with NGSIv2 and ngsi-ld and move all of the sinks that works with NGSI-LD, do you consider that it is a good approach? can you provide us additional guidelines for the correct packaging of the new code?

On the other hand, one of the main reasons for keeping separate the NGSIv2 from NGSI-LD is because of the way of how the NGSI-LD objects are built. This makes us also change the base Sink for working with the NGSI-LD object not with NGSIv2. The same thing happens with the batching mechanism. The current NGSIBatch only receives NGSIEvents (v2 objects). Thus, we can not use that class with NGSI-LD objects. The things that both versions share are very simple things like NGSI constants, NGSI charsets, etc. As I explained before that is why solutions like extends from NGSIv2 sinks are not working in this case, I mean that NGSILDPostgrsqlSink can not extend form NGSIPostgrsqlSink because the persistBacth method of the last one only allows to persists NGSIv2 objects.

Finally, we want to know your point of view about this and If it is possible to get feedback from you before to create the pull request.

Thanks, a lot for your help.

[fgalan]

Having merged PR #1885, should this issube be closed @anmunoz pls? Thx!

[anmunoz]

Yes, thanks for the guidance and accepting this contribution