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

Influxdb connector #1680

Merged
merged 95 commits into from
Jun 28, 2019
Merged

Influxdb connector #1680

merged 95 commits into from
Jun 28, 2019

Conversation

gkatzioura
Copy link
Contributor

@gkatzioura gkatzioura commented May 5, 2019

Purpose

This is the Alpakka connector for InfluxDB

References

References #1054

Changes

Added the connector for InfluxDB. Added InfluxDB on docker compose configuration.

Background Context

Commits are based on the source code of existing connectors like Elasticsearch and OrientDB.

Sources are created through queries. Inserts can be done both through models using the InfluxDB mapper or through Points. Batch operations are possible and enabled.

@gkatzioura
Copy link
Contributor Author

gkatzioura commented May 5, 2019

I get a failure both on the compatibility check (it also happens on master). Any help on fixing it, it will be greatly appreciated

[error] (awslambda / mimaReportBinaryIssues) akka-stream-alpakka-awslambda: Binary compatibility check failed!
[error] (influxdb / mimaPreviousClassfiles) sbt.librarymanagement.ResolveException: unresolved dependency: com.lightbend.akka#akka-stream-alpakka-influxdb_2.12;1.0-M3: not found
[error] (dynamodb / mimaReportBinaryIssues) akka-stream-alpakka-dynamodb: Binary compatibility check failed!
[error] (reference / mimaPreviousClassfiles) sbt.librarymanagement.ResolveException: unresolved dependency: com.lightbend.akka#akka-stream-alpakka-reference_2.12;1.0-M3: not found

@ennru ennru added the p:new label May 6, 2019
@ennru
Copy link
Member

ennru commented May 6, 2019

Thank you for this PR.

You may switch off the MiMa check for the new module with .disablePlugins(MimaPlugin). The other problems you mention don't show for me or on Travis.

@ennru
Copy link
Member

ennru commented May 6, 2019

Please add the new module to .travis as well.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Great, I think it is better to keep it simple and focus on the most crucial use cases. This is almost there but needs documentation.

* INTERNAL API
*/
@InternalApi
private[influxdb] final class InfluxDbRawSourceStage(query: Query, influxDB: InfluxDB)
Copy link
Member

Choose a reason for hiding this comment

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

I believe InfluxDbSourceLogic and InfluxDbRawSourceStage should have a common base class as they share most of the code.

val messages = grab(in)
if (messages.nonEmpty) {

influxDB.enableBatch(BatchOptions.DEFAULTS)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be important to make these settings configurable?

Copy link
Contributor Author

@gkatzioura gkatzioura Jun 14, 2019

Choose a reason for hiding this comment

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

This was removed as explained here

influxDB.enableBatch(BatchOptions.DEFAULTS)
write(messages)
val writtenMessages = messages.map(m => new InfluxDbWriteResult(m, None))
influxDB.close()
Copy link
Member

Choose a reason for hiding this comment

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

This API call is a bit surprising, but that might be just the way it is. Does this replace the disableBatch call which the API docs of enableBatch require?

Copy link
Contributor Author

@gkatzioura gkatzioura Jun 14, 2019

Choose a reason for hiding this comment

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

@ennru
Exactly. Once close() is called the batch is disabled and the messages are send to the database.
However this is no longer needed and It shall be removed.
I extracted the functionality, that the driver provided with batching enabled, and is currently executed by the connector itself.
This means that on a onPush call, the messages are written to the database in one http request.
Therefore the user has control over the batch size and the delay through the connector without the need to be aware of the driver internals.

@ennru
Copy link
Member

ennru commented Jun 25, 2019

@gkatzioura This looks good now, I propose we'll merge it and mark it as "API may change" so we keep the door open to apply more of @adkafka's suggestions.
We require documentation, though.
It would be perfect to get this in before we release Alpakka 1.0.3 on 2019-07-03.

@probot-autolabeler probot-autolabeler bot added dependency-change For PRs changing the version of a dependency. documentation labels Jun 27, 2019
@gkatzioura
Copy link
Contributor Author

gkatzioura commented Jun 27, 2019

@ennru
Just added the documentation and also added the @ApiMayChange annotation.
The api will indeed change apart from extra recommendations due to a major release on InfluxDB 2.0. Is there any faq on handling and contributing to api changes?

@ennru
Copy link
Member

ennru commented Jun 28, 2019

Thank's for the heads-up about the new version. I'll put a not in the documentation about both "API may change" and the upcoming InfluxDB version.

@ennru ennru added this to the 1.1.0 milestone Jun 28, 2019
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru changed the title Influxdb alpakka connector Influxdb connector Jun 28, 2019
@ennru ennru merged commit cd12632 into akka:master Jun 28, 2019
@ennru
Copy link
Member

ennru commented Jun 28, 2019

Thank you so much for this effort! We've seen the interest for InfluxDB support for a while, but didn't get to look into it. Highly appreciated.

@gkatzioura
Copy link
Contributor Author

gkatzioura commented Jun 28, 2019

@ennru
Thank you for merging!
I am keeping a close eye on the changes that occur. The version 2.0 is going to be huge, with extra functionality so I will be back with some contributions.

cheleb pushed a commit to cheleb/alpakka that referenced this pull request Jul 5, 2019
A new connector to stream data to and from InfluxDB.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-change For PRs changing the version of a dependency. documentation p:influxdb p:new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants