You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by ajayydv <gi...@git.apache.org> on 2016/07/13 20:12:36 UTC
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
GitHub user ajayydv opened a pull request:
https://github.com/apache/incubator-metron/pull/188
METRON-227 (Time based flushing)
METRON-227 (Time based flushing)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ajayydv/incubator-metron METRON-227
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-metron/pull/188.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #188
----
commit 6f091253f3d2716d0c286b2d3a232cd93a4872d2
Author: ajayydv <aj...@gmail.com>
Date: 2016-07-13T20:11:37Z
METRON-227 (Time based flushing)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/incubator-metron/pull/188
Any new comments or actions for this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/188
-1. Upon closer review, I find that there is no timer thread or similar mechanism implemented in this patch. Thus, the patch only works if there is a steady trickle of new tuples to trigger the time comparison. But if a burst of tuples are enqueued, and then no more tuples happen for a long time, the queue will languish without flushing until another tuple triggers the time comparison, potentially much later than the desired timeout.
@mmiklavc 's suggestion to use Tick Tuples instead is precisely the correct answer, as it avoids the considerable complexity of implementing a (robust) timer thread.
Since this PR has been dormant since July, and the contributor has not responded to queries on the Jira, I will take over the ticket, unless a more senior member of the community wishes to do so.
I recommend that this PR be closed without committing, and I will do the related work in the context of METRON-322, which I am also assigning to. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by cthistle <gi...@git.apache.org>.
Github user cthistle closed the pull request at:
https://github.com/apache/incubator-metron/pull/188
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/188
Michael, I'm in the process of reviewing this, and also pinging Ajay Yadav, in the context of METRON-322, to confirm he is not still active on this issue. Assuming he isn't, I will change this implementation to use Storm Tick Tuples, as this seems clearly the right path to follow.
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r70704170
--- Diff: metron-platform/metron-integration-test/src/main/config/zookeeper/global.json ---
@@ -7,10 +7,12 @@
"solr.collection": "metron",
"solr.numShards": 1,
"solr.replicationFactor": 1,
+ "flushIntervalInMs": "20000",
+ "flushOnTimeout": "false",
--- End diff --
Do we need both parameters? If we have a flush interval set, we should infer that we want to do a time-based flush, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by ajayydv <gi...@git.apache.org>.
Github user ajayydv commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r71033393
--- Diff: metron-platform/metron-integration-test/src/main/config/zookeeper/global.json ---
@@ -7,10 +7,12 @@
"solr.collection": "metron",
"solr.numShards": 1,
"solr.replicationFactor": 1,
+ "flushIntervalInMs": "20000",
+ "flushOnTimeout": "false",
--- End diff --
Agree that flushIntervalInMs is dependent on other flag but if we think from user perspective both have different meaning.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r71034703
--- Diff: metron-platform/metron-integration-test/src/main/config/zookeeper/global.json ---
@@ -7,10 +7,12 @@
"solr.collection": "metron",
"solr.numShards": 1,
"solr.replicationFactor": 1,
+ "flushIntervalInMs": "20000",
+ "flushOnTimeout": "false",
--- End diff --
What would you do if the flush flag were set to true and the flushIntervalInMs flag is unset?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r70704522
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/writer/BulkWriterComponent.java ---
@@ -103,10 +115,21 @@ public void write( String sensorType
}
messageList.add(message);
- if (tupleList.size() < batchSize) {
- sensorTupleMap.put(sensorType, tupleList);
- sensorMessageMap.put(sensorType, messageList);
- } else {
+ if(configurations.getGlobalConfig()!=null&&configurations.getGlobalConfig().get(Constants.TIME_FLUSH_FLAG)!=null)
--- End diff --
I'd recommend saving off the global config in a member variable and reusing that reference. It's possible the configuration will get updated between calls to `configurations.getGlobalConfig()` in zookeeper and therefore you'll get two separate configs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r70703025
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/writer/BulkWriterComponent.java ---
@@ -34,21 +34,37 @@
import java.util.function.Function;
public class BulkWriterComponent<MESSAGE_T> {
- public static final Logger LOG = LoggerFactory
- .getLogger(BulkWriterComponent.class);
+ public static final Logger LOG = LoggerFactory.getLogger(BulkWriterComponent.class);
private Map<String, Collection<Tuple>> sensorTupleMap = new HashMap<>();
private Map<String, List<MESSAGE_T>> sensorMessageMap = new HashMap<>();
private OutputCollector collector;
private boolean handleCommit = true;
private boolean handleError = true;
+ private Long lastFlushTime;
+ private Long flushIntervalInMs;
+ private boolean flush = false;
+
+
public BulkWriterComponent(OutputCollector collector) {
this.collector = collector;
+ this.lastFlushTime = System.currentTimeMillis();
+ this.flush = false;
}
public BulkWriterComponent(OutputCollector collector, boolean handleCommit, boolean handleError) {
this(collector);
this.handleCommit = handleCommit;
this.handleError = handleError;
+ this.lastFlushTime = System.currentTimeMillis();
+ this.flush = false;
+ }
+
+ public void setFlush(boolean flush) {
--- End diff --
Since you're deciding whether to flush based on variables in the global config, we don't need member variables and setters for `flush` and `flushInterval`. I don't like setting a member variable in the class every time we call write and we don't need it, they can be local variables to write
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by ajayydv <gi...@git.apache.org>.
Github user ajayydv commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r71033420
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/writer/BulkWriterComponent.java ---
@@ -34,21 +34,37 @@
import java.util.function.Function;
public class BulkWriterComponent<MESSAGE_T> {
- public static final Logger LOG = LoggerFactory
- .getLogger(BulkWriterComponent.class);
+ public static final Logger LOG = LoggerFactory.getLogger(BulkWriterComponent.class);
private Map<String, Collection<Tuple>> sensorTupleMap = new HashMap<>();
private Map<String, List<MESSAGE_T>> sensorMessageMap = new HashMap<>();
private OutputCollector collector;
private boolean handleCommit = true;
private boolean handleError = true;
+ private Long lastFlushTime;
+ private Long flushIntervalInMs;
+ private boolean flush = false;
+
+
public BulkWriterComponent(OutputCollector collector) {
this.collector = collector;
+ this.lastFlushTime = System.currentTimeMillis();
+ this.flush = false;
}
public BulkWriterComponent(OutputCollector collector, boolean handleCommit, boolean handleError) {
this(collector);
this.handleCommit = handleCommit;
this.handleError = handleError;
+ this.lastFlushTime = System.currentTimeMillis();
--- End diff --
agree
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/188
There is one downside that I see, but I think it's acceptable. We currently keep configuration around the indexing in zookeeper so that the topology configuration can be adjusted on the fly in a running topology. This would externalize that and keep the tick frequency in flux, so to change it would require a topology restart.
I think that this is acceptable, but it's worth pointing out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/188
Great contribution, thanks! I had a few comments, but good job. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by ajayydv <gi...@git.apache.org>.
Github user ajayydv commented on the issue:
https://github.com/apache/incubator-metron/pull/188
added test case
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/incubator-metron/pull/188#discussion_r70704088
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/writer/BulkWriterComponent.java ---
@@ -34,21 +34,37 @@
import java.util.function.Function;
public class BulkWriterComponent<MESSAGE_T> {
- public static final Logger LOG = LoggerFactory
- .getLogger(BulkWriterComponent.class);
+ public static final Logger LOG = LoggerFactory.getLogger(BulkWriterComponent.class);
private Map<String, Collection<Tuple>> sensorTupleMap = new HashMap<>();
private Map<String, List<MESSAGE_T>> sensorMessageMap = new HashMap<>();
private OutputCollector collector;
private boolean handleCommit = true;
private boolean handleError = true;
+ private Long lastFlushTime;
+ private Long flushIntervalInMs;
+ private boolean flush = false;
+
+
public BulkWriterComponent(OutputCollector collector) {
this.collector = collector;
+ this.lastFlushTime = System.currentTimeMillis();
+ this.flush = false;
}
public BulkWriterComponent(OutputCollector collector, boolean handleCommit, boolean handleError) {
this(collector);
this.handleCommit = handleCommit;
this.handleError = handleError;
+ this.lastFlushTime = System.currentTimeMillis();
--- End diff --
These guys aren't necessary because you're calling the constructor on line 55 which sets them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/188
@mmiklavc That is absolutely what the tick tuple was designed for. What are the pro/con versus the current proposed approach?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/incubator-metron/pull/188
It is a much simpler design and it leverages Storm's built-in mechanisms for performing actions like DB flushes. Also, you don't have to worry about multi-threading in your bolt if you want to flush prior to your buffer being full when you haven't received a tuple in a while. e.g. I have 99 tuples and need 100 for a flush, but don't receive a tuple for n minutes - without using tick tuples, you either just sit and wait, add a thread that needs to check internally, or add another external bolt with a clock.
I'm not aware of any cons to using the TT approach. This is what tick tuples were built for.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/incubator-metron/pull/188
Opened https://issues.apache.org/jira/browse/INFRA-12959 for Apache Infra to close this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/188
Whoops, forgot, we should probably have a unit test addition given this functionality.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by james-sirota <gi...@git.apache.org>.
Github user james-sirota commented on the issue:
https://github.com/apache/incubator-metron/pull/188
I agree on the -1. There are additionally merge conflicts now with this pull request and I don't believe it's worthwhile to do the merge and change the implementation. It's easier to start over. Matt, the Jira as been re-assigned to you. Thank you for taking this over. Lets let this sit for 72 hours and if no one objects we will cancel this pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #188: METRON-227 (Time based flushing)
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/incubator-metron/pull/188
How about using Storm's built-in tick tuples for this?
https://groups.google.com/forum/#!msg/storm-user/8addaQm3OT4/0OQfSgQkRwEJ
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---