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.

Sign up for GitHub

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

Jump to bottom

Pipeline register hooks + hash fix #6677

Merged
merged 1 commit into from Feb 14, 2017
Merged

Pipeline register hooks + hash fix #6677

merged 1 commit into from Feb 14, 2017

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Feb 9, 2017 •
edited

This PR does three things:

  1. It adds hooks for pipeline lifecycle events (and adds an LIR object to all pipeline objects)
  2. It fixes a bug where sometimes if statements would create invalid graph objects during their construction.
  3. It also fixes a bug where configs using strings as regex rvalues would break. Ex: [foo] =~ "bar"

@andrewvc andrewvc requested a review from ph February 9, 2017 17:51
andrewvc
andrewvc commented Feb 9, 2017
View reviewed changes
logstash-core/lib/logstash/compiler/lscl.rb
Selector,
LogStash::Compiler::LSCL::AST::RegExpOperator,
LogStash::Compiler::LSCL::AST::RegExp,
LogStash::Compiler::LSCL::AST::String # Strings work as rvalues! :p
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 was a shock.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was shocked too.

andrewvc
andrewvc commented Feb 9, 2017
View reviewed changes
logstash-core/src/main/java/org/logstash/config/ir/expression/Expression.java
@@ -7,7 +7,7 @@
import org.logstash.config.ir.SourceComponent;
import org.logstash.config.ir.SourceMetadata;

/**
/*
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 isn't really a javadoc

@andrewvc andrewvc force-pushed the pipeline_register_hash_fix branch 2 times, most recently from 2df5dc4 to 40f2e9c Compare February 9, 2017 17:55
andrewvc
andrewvc commented Feb 9, 2017
View reviewed changes
.gitignore Outdated
@@ -37,9 +37,6 @@ qa/Gemfile.lock
*.iws
*.iml
.gradle
**/.gradle/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These aren't in master and shouldn't be in LIR

ph
ph suggested changes Feb 13, 2017
View reviewed changes
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Can you explain the asserts you are doing at https://github.com/elastic/logstash/pull/6677/files#diff-f9fdf28f867b25593d8af2d613584817R147

Shouldn't we check the branch hash values and compare them?

logstash-core/lib/logstash/compiler/lscl.rb
Selector,
LogStash::Compiler::LSCL::AST::RegExpOperator,
LogStash::Compiler::LSCL::AST::RegExp,
LogStash::Compiler::LSCL::AST::String # Strings work as rvalues! :p
Copy link
Contributor

Choose a reason for hiding this comment

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

I was shocked too.

logstash-core/lib/logstash/compiler/lscl.rb
# Maybe we really shouldn't handle these anymore...
if regexp.class == org.logstash.config.ir.expression.ValueExpression
regexp = jdsl.eRegex(regexp.get)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I live in a world where I do prefer explicit, I wasn't aware that we were handle theses case.
Can we create an issue that target 6.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#6697

logstash-core/lib/logstash/instrument/metric_store.rb
@@ -218,7 +218,7 @@ def get_recursively(key_paths, map, new_hash)
key_candidates = extract_filter_keys(key_paths.shift)

key_candidates.each do |key_candidate|
raise MetricNotFound, "For path: #{key_candidate}" if map[key_candidate].nil?
raise MetricNotFound, "For path: #{key_candidate}. Map keys: #{map.keys}" if map[key_candidate].nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I hit a weird bug, this made it easier to discover :)

logstash-core/src/test/java/org/logstash/config/ir/graph/GraphTest.java Outdated

Graph g = imperative.toGraph();
g.validate(); // Double validate just because that's the point of this test
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand the assert in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ph well, there's no assert here because validatewill throw an InvalidIRException if it fails.

@ph
Copy link
Contributor

ph commented Feb 13, 2017

Also this PR superseed this one #6517 ? correct?

@ph
Copy link
Contributor

ph commented Feb 13, 2017

oops unit test are failling!

@andrewvc
Fix graph hashing
2d2cd27 
Add UniversalPlugin hooks for pipeline events
@andrewvc andrewvc mentioned this pull request Feb 14, 2017
Closed
@andrewvc andrewvc merged commit ebc54ad into elastic:lir Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ph ph ph approved these changes

Assignees

@ph ph

Labels
bug enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andrewvc @ph