Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[cygnus-ngsi-ld] New cygnus agent for NGSLD support #1885

Merged
merged 23 commits into from
Jul 7, 2020
Merged

[cygnus-ngsi-ld] New cygnus agent for NGSLD support #1885

merged 23 commits into from
Jul 7, 2020

Conversation

anmunoz
Copy link
Contributor

@anmunoz anmunoz commented Jun 9, 2020

This PR is related to issue #1875. From UPM we have been working with FIWARE Foundation for trying to define the first development for providing NGSI-LD support to Cygnus. This pull request incorporates this new cygnus agent called cygnus-ngsi-ld. 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.This PR replaces the PR #1879 to amend the packaging and provide a new bundle

@anmunoz anmunoz mentioned this pull request Jun 9, 2020
Closed
@fgalan
Copy link
Member

fgalan commented Jun 10, 2020

Feedback provided in the old PR (to be checked in this one in the review process)

Taking into account the feedback provided in issue #1875, we think it should be better to package the new notifications handler and sinks in a new "bundle" (not sure of the term... some parts of the documentation named this "sub-module", eg. https://github.com/telefonicaid/fiware-cygnus#install).

I mean, a new subdirectory cygnus-ngsi-ld/ with a parallel structure and way of working as the existing cygnus-ngsi/, relyging in cygnus-common/ for the common parts (have a look on how cygnus-ngsi includes cygnus-common in its pom.xml file https://github.com/telefonicaid/fiware-cygnus/blob/master/cygnus-ngsi/pom.xml#L70, cygnus-ngsi-ld should use a similar approach).

Note that all Cygnus bundles/sub-modules are supposed to have RPM generation artifacts (in spec/SPECS subdirectory). However, with this one I think that just providing docker container generation artifacts is enough (that stuff shoud be in docker/cygnus-ngsi-ld).

Finally, with regards to documentation, the place is doc/cygnus-ngsi-ld. New files should be included in a new "section" of the mkdocs.yml file.

fgalan
fgalan reviewed Jun 10, 2020
View reviewed changes
cygnus-common/pom.xml Outdated Show resolved Hide resolved
fgalan
fgalan reviewed Jun 10, 2020
View reviewed changes
mkdocs.yml Show resolved Hide resolved
@fgalan
Copy link
Member

fgalan commented Jun 16, 2020

It seems there is a small conflict in CHANGES_NEXT_RELEASE file. Please, fix it.

anmunoz and others added 4 commits June 16, 2020 18:13
@anmunoz @fgalan
Update doc/cygnus-ngsi-ld/user_and_programmer_guide/adding_new_sink.md
246d23b 
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
@anmunoz
Update CHANGES_NEXT_RELEASE
58ac9e6
@anmunoz
Merge branch 'master' into master
9d14528
@anmunoz @fgalan
Update doc/cygnus-ngsi-ld/flume_extensions_catalogue/ngsi_rest_handle…
b2083b2 
…r.md

Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
fgalan
fgalan reviewed Jun 18, 2020
View reviewed changes
CHANGES_NEXT_RELEASE Outdated
@@ -2,4 +2,6 @@
[cygnus-ngsi][CKANSink] New datamodel for CKAN (dm-by-entity-id) implementing mapping: subservice -> org, entityId -> dataset, entityId -> resource (#1792)
[cygnus-ngsi][PostgisSQLSink, PostgreSQLSink] Add dm-by-entity-database and dm-by-entity-database-schema.
[cygnus-ngsi][Generic Aggregation] If name mappings is enabled, then the aggregation will take all attributes from the mapped element on the event.
[cygnus-common][pom.xml] Update Hive-jdbc dependency from version 2.3.4 to version 3.1.2
Copy link
Member

Choose a reason for hiding this comment

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

Checking this with @IvanHdzC and @AlvaroVega , we wonder why this upgrade is needed. I mean, it is just a routinary package version upgrade? Or is there any hive-jdbc functionality/fix need by cygnus-ngsi-ld that exists in 3.1.2 but not in 2.3.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, any update on this?

Copy link
Member

Choose a reason for hiding this comment

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

We are still waiting for the answers to these questions:

  • Is it just a routinary package version upgrade?
  • Or is there any hive-jdbc functionality/fix need by cygnus-ngsi-ld that exists in 3.1.2 but not in 2.3.4?

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

(In addition, an upgrade to CHANGES_NEXT_RELEASE in master due to recent 2.2.0 release yesterday has caused a minor conflict in this PR. Could you solve it so the PR gets mergeable again, please? Thanks!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 917bf6a

Copy link
Member

Choose a reason for hiding this comment

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

What about the pending questions @anmunoz ? Any update on them, pls? Thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, is just a routinary update, when we compile cygnus from sources we get an error about this dependency and the code for hive Sink, that is the reason why we decided to update it. Cygnus-ngsi-ld currently don't work with hive so that is not a necessary dependency for this development.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the answer!

Taking into account that change is not related to the purpose of the PR itself (cygnus-ngsi-ld) and that we (@AlvaroVega @IvanHdzC @fgalan ) have some doubts about compatibility (we haven't been able to trace the 2.3.4 to 3.1.2 changelog in that dependency) please move that change in pom.xml (and associated CHANGES_NEXT_RELEASE entry) to a separate PR so we can continue on it and this PR (the one in cygnus-ld) can be merged now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 1dc0f55 and 75c3a8d

fgalan
fgalan reviewed Jun 24, 2020
View reviewed changes
CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved
@anmunoz @fgalan
Update CHANGES_NEXT_RELEASE
6a012af 
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
fgalan
fgalan approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@fgalan
Copy link
Member

fgalan commented Jun 24, 2020

Waiting to travis to pass on the final commit (that's weird... travis should start automatically if the PR doesn't have any conflict, which is the case).

@fgalan
Copy link
Member

fgalan commented Jun 24, 2020

Finally, travis ran (I manually triggered it).

Looking to its results (on commit 6a012af ) I see

Tests run: 773, Failures: 0, Errors: 0, Skipped: 0

which is the same number we have now in master branch (on commit 9c2b3a2). So I guess that the tests included in this PR for cygnus-ngsi-ld are not being passed by travis.

I think it's a matter of modifying test.sh to add them, in a similar way cygnus-ngsi is included. @anmunoz could you manage, please?

@anmunoz
Copy link
Contributor Author

anmunoz commented Jun 26, 2020

Yes, of course, I added that in a9516ed

@fgalan
Copy link
Member

fgalan commented Jun 29, 2020

Yes, of course, I added that in a9516ed

Thanks!

However, it seems that after that modification, tests have broken. Travis report:

[ERROR] Failed to execute goal on project cygnus-ngsi-ld: Could not resolve dependencies for project com.telefonica.iot:cygnus-ngsi-ld:jar:2.1.0_SNAPSHOT: The following artifacts could not be resolved: com.telefonica.iot:cygnus-common:jar:2.1.0_SNAPSHOT, es.aytosantander.smartcity:ArcgisUtils:jar:0.0.1-SNAPSHOT: Could not find artifact com.telefonica.iot:cygnus-common:jar:2.1.0_SNAPSHOT in sonatype-snapshots (https://oss.sonatype.org/content/repositories/snapshots/) -> [Help 1]

Could you have a look, please?

@fgalan
Copy link
Member

fgalan commented Jul 3, 2020

Yes, of course, I added that in a9516ed

Thanks!

However, it seems that after that modification, tests have broken. Travis report:

[ERROR] Failed to execute goal on project cygnus-ngsi-ld: Could not resolve dependencies for project com.telefonica.iot:cygnus-ngsi-ld:jar:2.1.0_SNAPSHOT: The following artifacts could not be resolved: com.telefonica.iot:cygnus-common:jar:2.1.0_SNAPSHOT, es.aytosantander.smartcity:ArcgisUtils:jar:0.0.1-SNAPSHOT: Could not find artifact com.telefonica.iot:cygnus-common:jar:2.1.0_SNAPSHOT in sonatype-snapshots (https://oss.sonatype.org/content/repositories/snapshots/) -> [Help 1]

Could you have a look, please?

@anmunoz any update on this, please? Thx!

@anmunoz
Copy link
Contributor Author

anmunoz commented Jul 6, 2020

Sorry for the delay, we removed the ArcGIS dependencies because we prioritize the development of other sinks first e6493ce

@fgalan
Copy link
Member

fgalan commented Jul 7, 2020

LGTM to added commits fromm last LGTM (6a012af...6a91d97)

@fgalan fgalan merged commit 8c63831 into telefonicaid:master Jul 7, 2020
@fgalan fgalan mentioned this pull request Jul 7, 2020
Closed
@fgalan fgalan mentioned this pull request Sep 7, 2020
Merged
@mapedraza mapedraza mentioned this pull request Sep 7, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgalan fgalan fgalan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anmunoz @fgalan