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.

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

Fix/sql backend destination #2007

Merged
merged 18 commits into from
Mar 29, 2021
Merged

Fix/sql backend destination #2007

merged 18 commits into from
Mar 29, 2021

Conversation

IvanHdzC
Copy link
Contributor

@IvanHdzC IvanHdzC commented Mar 25, 2021

fix for #2004

You can find test results on the attached files.

agent.txt
cygnus.txt

@AlvaroVega
Copy link
Member

A CNR update is needed

@@ -34,37 +34,38 @@

/**
* Creates a table, given its name, if not exists in the given database.
* @param destination
* @param dataBase
* @param tableName
Copy link
Member

Choose a reason for hiding this comment

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

missing param schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f30b89d

* @param tableName
* @param maxRecords
* @throws com.telefonica.iot.cygnus.errors.CygnusRuntimeError
* @throws com.telefonica.iot.cygnus.errors.CygnusPersistenceError
*/
void capRecords(String destination, String tableName, long maxRecords) throws CygnusRuntimeError, CygnusPersistenceError;
void capRecords(String dataBase, String tableName, long maxRecords) throws CygnusRuntimeError, CygnusPersistenceError;
Copy link
Member

Choose a reason for hiding this comment

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

schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is only implemented by MySQL sink, That's why I didn't add the schema param. Do you thing It should be included?

@AlvaroVega
Copy link
Member

AlvaroVega commented Mar 25, 2021

Still duplicated sentence:

aggregator.setSchemeName(buildSchemaName(aggregator.getService(), aggregator.getServicePathForNaming()));

@IvanHdzC
Copy link
Contributor Author

@@ -0,0 +1,18 @@
package com.telefonica.iot.cygnus.backends.sql;
Copy link
Member

Choose a reason for hiding this comment

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

License header in all new files, pls :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8ef7b73

@IvanHdzC IvanHdzC changed the title [WIP]Fix/sql backend destination Fix/sql backend destination Mar 25, 2021
Comment on lines +1 to +3
[cygnus-ngsi] [PostgisSink] Fix PostgreSQL encoding when enabled.
[cygnus-ngsi] [MysqlSink, PostgisSink, PosgreSQLSink] Update sinks to initialize persistance backend objects without database. (#2004)
[cygnus-common] Remove database initialization on constructors. (#2004)
Copy link
Member

Choose a reason for hiding this comment

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

This PR has no any impact in documentation?

Just to check...

Copy link
Member

Choose a reason for hiding this comment

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

It's just a bugfix, do not introduce a new behavior

Copy link
Member

Choose a reason for hiding this comment

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

NTC

if (buildDBName(null) != null) {
createPersistenceBackend(postgisHost, postgisPort, postgisUsername, postgisPassword, maxPoolSize, buildDBName(null), postgisOptions);
}
createPersistenceBackend(postgisHost, postgisPort, postgisUsername, postgisPassword, maxPoolSize, postgisOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Note that ngsi-ld sinks like this are not fully aligned with ngsiv2 sinks and maybe are not received latest features, bugfixes and refactors.

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

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

@AlvaroVega AlvaroVega merged commit f85dd5d into master Mar 29, 2021
@AlvaroVega AlvaroVega deleted the fix/sql_backend_destination branch March 29, 2021 07:58
@fgalan
Copy link
Member

fgalan commented Mar 29, 2021

@anmunoz this PR fixes some sinks in cygnus-ngsi that also are being used in cygnus-ngsi-ld. I'm telling you, just in case you want also to apply similar fixes to them.

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.

None yet

4 participants