Skip to content
This repository has been archived by the owner on Nov 11, 2022. It is now read-only.

Contrib module for natsio #61

Closed
wants to merge 12 commits into from

Conversation

tyagihas
Copy link

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@tyagihas
Copy link
Author

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot
Copy link

CLAs look good, thanks!

Run tests with Maven. The following command will launch a producer and consumer job respectively.<BR>
The producer publishes messages and the consumer will receive them via NATS server.
```bash
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

merge conflicts?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@dhalperi dhalperi self-assigned this Sep 29, 2015
@@ -0,0 +1,58 @@
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to follow the model set out in the contrib join library. Specifically

  • Artifact naming
  • License information
  • Compiler source and target versions (1.7, probably)
  • optionally, Checkstyle

Also,

  • review dependencies

} catch (InterruptedException e) {
e.printStackTrace();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a NullPointerException can happen here when the thread is interrupted. This seems like the wrong handling of the InterruptedException?

Copy link
Author

Choose a reason for hiding this comment

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

Agree. I will change to throw an IOException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is for a source/sink, what you want to do is effectively tell the caller above that you were closed by an interrupt.

Throwing something like ClosedByInterruptException seems to make sense after cleaning up any resources.

We will fail the task, and restart the write operation again (on this or another worker).

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that makes more sense. I will change it to throw ClosedByInterruptException.

@dhalperi
Copy link
Contributor

Hi @tyagihas:

I am encountering the following error installing this module:

[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/dhalperi/tyagihas/DataflowJavaSDK/contrib/natsio/src/main/java/com/google/cloud/dataflow/contrib/nats
io/example/NatsIOBench.java:[14,48] cannot find symbol
  symbol:   class NatsIOTest
  location: package com.google.cloud.dataflow.contrib.natsio
[ERROR] /home/dhalperi/tyagihas/DataflowJavaSDK/contrib/natsio/src/main/java/com/google/cloud/dataflow/contrib/nats
io/example/NatsIOBench.java:[26,67] cannot find symbol
  symbol:   class NatsIOTest
  location: class com.google.cloud.dataflow.contrib.natsio.example.NatsIOBench
[INFO] 2 errors 
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 5.870s
[INFO] Finished at: Mon Oct 19 21:13:44 UTC 2015
[INFO] Final Memory: 19M/56M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:compile (default-compile) on proj
ect google-cloud-dataflow-java-contrib-natsio: Compilation failure: Compilation failure:
[ERROR] /home/dhalperi/tyagihas/DataflowJavaSDK/contrib/natsio/src/main/java/com/google/cloud/dataflow/contrib/nats
io/example/NatsIOBench.java:[14,48] cannot find symbol
[ERROR] symbol:   class NatsIOTest
[ERROR] location: package com.google.cloud.dataflow.contrib.natsio
[ERROR] /home/dhalperi/tyagihas/DataflowJavaSDK/contrib/natsio/src/main/java/com/google/cloud/dataflow/contrib/nats
io/example/NatsIOBench.java:[26,67] cannot find symbol
[ERROR] symbol:   class NatsIOTest
[ERROR] location: class com.google.cloud.dataflow.contrib.natsio.example.NatsIOBench
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

Here's a log of the important commands I have run, on a fresh Ubuntu 15.04 VM:

I suspect you want the NatsIOBench class to be in the test rather than in main. Normally, code in main should not depend on code in test.

@dhalperi
Copy link
Contributor

Assigning back to you for now, please reassign to me once fixed :).

@dhalperi dhalperi removed their assignment Oct 19, 2015
@dhalperi
Copy link
Contributor

Hmm, can't re-assign to you, so please just bump the request once fixed. Thanks.

@dhalperi dhalperi self-assigned this Oct 19, 2015
@tyagihas
Copy link
Author

Fixed the class reference. NatsIOBench shouldn't include reference to NatsIOTest.
I also added some default test properties for maven test and only three parameters are mandatory for minimum testing.

mvn install -Dproject="project id" -DstagingLocation="bucket" -Dnats.servers=nats://"GCE private ip":4222

@dhalperi
Copy link
Contributor

I ran the job as follows: mvn -Dproject="$PROJECT" -DstagingLocation="$STAGING" -D"nats.servers=nats://0.0.0.0:4222" test

Two Dataflow jobs start in my project, but they never make any progress and have not stopped after at least 10 minutes of running. In Cloud Logging, I see some very confusing logs, attached below.

It looks like some of the code needs to use a org.slf4h.Logger instead of System.err/out to report errors. Maybe this is coming from org.nats.Connection in this stack trace? https://github.com/tyagihas/java_nats/blob/master/src/main/java/org/nats/Connection.java#L215

This failure is somehow not failing the job, and I think it should.

ahpkiqggwt9

@dhalperi
Copy link
Contributor

Both jobs did fail after 15 minutes, I guess because it was retried 4 times.

```bash
% mvn test -Dtest=NatsIOTest#publishSubscribe -DstagingLocation=gs://<bucket> ¥
-Dproject=<project id> -Dnats.servers=nats://<server>:4222 -Dnats.queue=queue1 ¥
-Dloop=30000 -Dinterval=0 -Dsubjects=test1 -Dconsumers=1 -Dproducers=1 -Dnats.maxRecords=20000 ¥
Copy link
Contributor

Choose a reason for hiding this comment

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

This symbol that is rendering as a Yen sign for me -- is this a Japanese keyboard version of \ to mean "continues on next line"? (if so, cool!)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's same meaning as backslash.

@dhalperi
Copy link
Contributor

dhalperi commented Nov 3, 2015

GitHub notifications don't come when patches are pushed; please comment once code is updated next.

Thanks!

@tyagihas
Copy link
Author

tyagihas commented Nov 5, 2015

Thank you for all the feedbacks. I've just pushed the latest code.

@tyagihas
Copy link
Author

Thank you for the comments. I just push a new commit.

@tyagihas
Copy link
Author

Hi @dhalperi

It's been a while since last update. Did you have time to look at the latest commit?


public NatsSource(String subject, Properties props) {
if (subject == null) {
throw new NullPointerException("subject");
Copy link
Contributor

Choose a reason for hiding this comment

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

would be simpler if you use Guava Preconditions:

import static com.google.common.base.Preconditions.checkNotNull;

....

checkNotNull(subject, "subject");
...

@dhalperi
Copy link
Contributor

No movement for 60 days since last review, so I'm closing.

Note that we are moving active development to Apache Beam (incubating). If you wish to resubmit, please read the Beam contribution guide and submit there.

@dhalperi dhalperi closed this Mar 30, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants