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

Issue 187: Add MQTT to Pravega bridge sample #188

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

claudiofahey
Copy link

Add MQTT to Pravega bridge sample.
A sample application reads events from MQTT and writes them to a Pravega stream.

@claudiofahey claudiofahey force-pushed the issue-187-mqtt-pravega-bridge branch 2 times, most recently from c736fd4 to 306d00b Compare April 18, 2019 17:12
@claudiofahey claudiofahey requested a review from fpj April 18, 2019 17:13
scenarios/mqtt-pravega-bridge/README.md Outdated Show resolved Hide resolved
scenarios/mqtt-pravega-bridge/README.md Outdated Show resolved Hide resolved
scenarios/mqtt-pravega-bridge/build.gradle Outdated Show resolved Hide resolved
scenarios/mqtt-pravega-bridge/build.gradle Outdated Show resolved Hide resolved
scenarios/mqtt-pravega-bridge/build.gradle Outdated Show resolved Hide resolved
@RaulGracia
Copy link
Contributor

@claudiofahey due to issues we had in previous releases merging develop into master, we have changed the release approach and the current development branch is dev. We plan to delete develop to avoid confusions, so could you please re-open this PR against dev? Thanks!

@claudiofahey claudiofahey changed the base branch from develop to dev August 13, 2019 21:44
@claudiofahey claudiofahey dismissed vijikarthi’s stale review August 13, 2019 21:48

suggested changes have been implemented

@claudiofahey claudiofahey removed the request for review from fpj August 13, 2019 21:49
@claudiofahey
Copy link
Author

This PR is ready for re-review.

Copy link
Contributor

@fpj fpj left a comment

Choose a reason for hiding this comment

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

It looks good. I only have a few small comments and questions.

@@ -0,0 +1,51 @@
package io.pravega.example.mqtt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header.

@@ -0,0 +1,82 @@
package io.pravega.example.mqtt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header.

@@ -0,0 +1,60 @@
package io.pravega.example.mqtt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header.

@@ -0,0 +1,56 @@
package io.pravega.example.mqtt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header.

@@ -0,0 +1,76 @@
package io.pravega.example.mqtt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing license header.

<?xml version="1.0" encoding="UTF-8"?>
<!--

Copyright (c) 2017 Dell Inc., or its subsidiaries. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove year from copyright statement.

@@ -0,0 +1,55 @@
plugins {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are adding license headers to such gradle files too, see pravega/pravega.

# Pravega Properties
controllerUri=tcp://127.0.0.1:9090
scope=examples
stream=mqtt-example
Copy link
Contributor

Choose a reason for hiding this comment

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

For a sample, writing to a single stream is ok, but in general, isn't it possible that we have use cases in which the same bridge writes to different streams, perhaps inferring the stream from the event data?

@fpj fpj removed the request for review from vijikarthi August 16, 2020 15:41
Copy link
Contributor

@RaulGracia RaulGracia left a comment

Choose a reason for hiding this comment

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

Looks good, just few minor comments.


private void loadProperties(String confDir) throws Exception{
Properties prop = new Properties();
try (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In Pravega core, the style of these statements is in one line.


public class ApplicationMain {

private static Logger log = LoggerFactory.getLogger( ApplicationMain.class );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra spaces within parenthesis.

@@ -0,0 +1,39 @@
package io.pravega.example.mqtt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree on the license headers missing.

<?xml version="1.0" encoding="UTF-8"?>
<!--

Copyright (c) 2017 Dell Inc., or its subsidiaries. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here about removing year from header.

@RaulGracia
Copy link
Contributor

@fpj @claudiofahey do we want to merge this for 0.8?

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.

4 participants