Pier-Hugues Pellerin ph

Developer Program Member

Organizations

@Ouranosinc @elastic
ph commented on pull request elastic/logstash#3110
@ph

code lgtm, test ran, also tried @wiibaa's config with that PR with success.

ph commented on issue elastic/logstash#2376
@ph

@mkristian Thank you for hoping in this thread, will try to team with you to fix theses issues.

ph commented on issue elastic/logstash#3101
@ph

What is the content of the logstash.log, complete events, warning or error?

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

Looking good @purbon ! :)

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

Since I did a small check on the input I have also look at the output. I havent test them locally just did the core review.

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

I would decouple the building of the query from the query execution, this make it easier to unit test. def build_query(start_time) query = <<-QUERY …

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

thius could be replaced by a one liner nodes.empty? ? clazz.create(criteria) : nodes.first

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

remove one space after node.

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

Where are these methods used? (to_arr and has_child)

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

When defining temporary path or temporary files we should the Stud Library.

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

To be consistent in the style of this file I would add space after { and before }, all the other procs are defined like this.

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

Spaces?

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

Already 0.9.1, are you following Neo4J versioning?

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

resultset.map(&:e).to_a

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

Also, maybe we should create another folder to contains these support classes? I don't think they belong in the outputs or logstash namespace? If t…

ph commented on pull request logstash-plugins/logstash-output-neo4j#1
@ph

We can lazy require this library in the register method.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

Made small comments look good to me.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

Where is the to_arr and the has_child method used? I do not see reference of them in the code.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

plugin.run(logstash_queue)

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

If you are using the class or module in the describe block this will also correctly set the subject method with this value.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

The to_json method is using the json standard library and not our jrjackson library, It's better to use LogStash::Json.dump(object) to serialize ob…

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

I would replace this with rel.respond_to?(:labels) ? rel.labels : "Relationship"

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

This method is using the same name as the configuration option and I find that a bit confusing. I suggest we rename it to configure_scheduler.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

could be replaced with a one line. @schedule ? schedule(queue) : fetch(queue)

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

We can remove this require since we already do a lazy require in the register method.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

I think we should be consistent with our hash styling, we are still using hash rocket style.

ph commented on pull request logstash-plugins/logstash-input-neo4j#1
@ph

methods are public by default

ph closed pull request elastic/logstash-forwarder#445
@ph
Remove duplicated code
ph commented on issue elastic/logstash#3096
@ph

Are we planning to allow people to update the core within logstash?

@ph

@driskell I might have been a big direct in my last reply. I am sorry. Let me do a bit of explanation. The threadpool/timeout will help a bit for t…