[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user asfgit closed the pull request at: https://github.com/apache/metron/pull/831 ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user cestella commented on a diff in the pull request: https://github.com/apache/metron/pull/831#discussion_r160161491 --- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/templates/hdfs.properties.j2 --- @@ -0,0 +1,44 @@ +# Licensed to the Apache Software Foundation (ASF) under one --- End diff -- Yep ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r160161476
--- Diff:
metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/integration/IndexingIntegrationTest.java
---
@@ -197,9 +140,7 @@ public void test() throws Exception {
//assert that our input docs are equivalent to the output docs,
converting the input docs keys based
// on the field name converter
assertInputDocsMatchOutputs(inputDocs, docs,
getFieldNameConverter());
- assertInputDocsMatchOutputs(inputDocs, readDocsFromDisk(hdfsDir), x
-> x);
-} catch(Throwable e) {
- e.printStackTrace();
+ //assertInputDocsMatchOutputs(inputDocs, readDocsFromDisk(hdfsDir),
x -> x);
--- End diff --
Nope
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user merrimanr commented on a diff in the pull request: https://github.com/apache/metron/pull/831#discussion_r159322924 --- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/templates/hdfs.properties.j2 --- @@ -0,0 +1,44 @@ +# Licensed to the Apache Software Foundation (ASF) under one --- End diff -- Should this file be git ignored since it is copied in at build time? ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r159321881
--- Diff:
metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/integration/IndexingIntegrationTest.java
---
@@ -197,9 +140,7 @@ public void test() throws Exception {
//assert that our input docs are equivalent to the output docs,
converting the input docs keys based
// on the field name converter
assertInputDocsMatchOutputs(inputDocs, docs,
getFieldNameConverter());
- assertInputDocsMatchOutputs(inputDocs, readDocsFromDisk(hdfsDir), x
-> x);
-} catch(Throwable e) {
- e.printStackTrace();
+ //assertInputDocsMatchOutputs(inputDocs, readDocsFromDisk(hdfsDir),
x -> x);
--- End diff --
Is this comment intentional?
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r151190365
--- Diff:
metron-platform/metron-indexing/src/main/scripts/start_hdfs_topology.sh ---
@@ -0,0 +1,22 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+METRON_VERSION=${project.version}
+METRON_HOME=/usr/metron/$METRON_VERSION
+TOPOLOGY_JAR=metron-elasticsearch-$METRON_VERSION-uber.jar
+storm jar $METRON_HOME/lib/$TOPOLOGY_JAR org.apache.storm.flux.Flux
--remote $METRON_HOME/flux/indexing/batch/remote.yaml --filter
$METRON_HOME/config/hdfs.properties
--- End diff --
Yeah, I prefer to keep the scripts `hdfs` and `elasticsearch`
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r151190210
--- Diff:
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/indexing_commands.py
---
@@ -162,34 +164,77 @@ def start_indexing_topology(self, env):
self.__params.metron_principal_name,
execute_user=self.__params.metron_user)
-start_cmd_template =
"""{0}/bin/start_elasticsearch_topology.sh \
--s {1} \
--z {2}"""
-start_cmd =
start_cmd_template.format(self.__params.metron_home,
- self.__indexing_topology,
-
self.__params.zookeeper_quorum)
+start_cmd_template = """{0}/bin/start_hdfs_topology.sh"""
+start_cmd =
start_cmd_template.format(self.__params.metron_home)
Execute(start_cmd, user=self.__params.metron_user, tries=3,
try_sleep=5, logoutput=True)
else:
-Logger.info('Indexing topology already running')
+Logger.info('Batch Indexing topology already running')
-Logger.info('Finished starting indexing topology')
+Logger.info('Finished starting batch indexing topology')
-def stop_indexing_topology(self, env):
-Logger.info('Stopping ' + self.__indexing_topology)
+def start_random_access_indexing_topology(self, env):
+Logger.info('Starting ' + self.__random_access_indexing_topology)
--- End diff --
Definitely, @nickwallen I think we're thinking the same thing. This is
literally just the first baby step toward a broader vision of pluggable
writers. I think we probably want to start here with a discuss thread. Where
I was thinking of going next in here was merging the flux files into one and
pulling the configs from zookeeper, which would be a step towards your vision.
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user cestella commented on a diff in the pull request: https://github.com/apache/metron/pull/831#discussion_r151189633 --- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/configuration/metron-indexing-env.xml --- @@ -73,18 +98,31 @@ Indexing Update Column Family -indexing_workers +ra_indexing_workers +Number of Indexing Topology Workers --- End diff -- I should add, or some other 3rd option that I haven't anticipated. ;) ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user cestella commented on a diff in the pull request: https://github.com/apache/metron/pull/831#discussion_r151189543 --- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/configuration/metron-indexing-env.xml --- @@ -73,18 +98,31 @@ Indexing Update Column Family -indexing_workers +ra_indexing_workers +Number of Indexing Topology Workers --- End diff -- That sounds good. Which one would you guys prefer: * display names in the mpack of "Elasticsearch" and "HDFS" * variable prefixes of `es` and `hdfs` I think I'd prefer the first as I really have been trying to set us up to a world where we can make things pluggable (this is the first baby step). ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r150336506
--- Diff:
metron-platform/metron-indexing/src/main/scripts/start_hdfs_topology.sh ---
@@ -0,0 +1,22 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+METRON_VERSION=${project.version}
+METRON_HOME=/usr/metron/$METRON_VERSION
+TOPOLOGY_JAR=metron-elasticsearch-$METRON_VERSION-uber.jar
+storm jar $METRON_HOME/lib/$TOPOLOGY_JAR org.apache.storm.flux.Flux
--remote $METRON_HOME/flux/indexing/batch/remote.yaml --filter
$METRON_HOME/config/hdfs.properties
--- End diff --
Very true. I think you're right, @ottobackwards
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r150317133
--- Diff:
metron-platform/metron-indexing/src/main/scripts/start_hdfs_topology.sh ---
@@ -0,0 +1,22 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+METRON_VERSION=${project.version}
+METRON_HOME=/usr/metron/$METRON_VERSION
+TOPOLOGY_JAR=metron-elasticsearch-$METRON_VERSION-uber.jar
+storm jar $METRON_HOME/lib/$TOPOLOGY_JAR org.apache.storm.flux.Flux
--remote $METRON_HOME/flux/indexing/batch/remote.yaml --filter
$METRON_HOME/config/hdfs.properties
--- End diff --
I almost think that if it is going to be hdfs, we should call it hdfs. If
we have multiple topologies based on the batch indexer, each will end up with a
unique name anyways ( S3 indexer etc ).
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r150316808
--- Diff:
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/indexing_commands.py
---
@@ -162,34 +164,77 @@ def start_indexing_topology(self, env):
self.__params.metron_principal_name,
execute_user=self.__params.metron_user)
-start_cmd_template =
"""{0}/bin/start_elasticsearch_topology.sh \
--s {1} \
--z {2}"""
-start_cmd =
start_cmd_template.format(self.__params.metron_home,
- self.__indexing_topology,
-
self.__params.zookeeper_quorum)
+start_cmd_template = """{0}/bin/start_hdfs_topology.sh"""
+start_cmd =
start_cmd_template.format(self.__params.metron_home)
Execute(start_cmd, user=self.__params.metron_user, tries=3,
try_sleep=5, logoutput=True)
else:
-Logger.info('Indexing topology already running')
+Logger.info('Batch Indexing topology already running')
-Logger.info('Finished starting indexing topology')
+Logger.info('Finished starting batch indexing topology')
-def stop_indexing_topology(self, env):
-Logger.info('Stopping ' + self.__indexing_topology)
+def start_random_access_indexing_topology(self, env):
+Logger.info('Starting ' + self.__random_access_indexing_topology)
--- End diff --
I like this a lot. I would like to see the indexing be like the parses,
where you start with a name parameter. I think the flux files make it
difficult though. I would be +1 exploring this approach though, maybe a
feature branch?
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user nickwallen commented on a diff in the pull request: https://github.com/apache/metron/pull/831#discussion_r150315275 --- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/configuration/metron-indexing-env.xml --- @@ -73,18 +98,31 @@ Indexing Update Column Family -indexing_workers +ra_indexing_workers +Number of Indexing Topology Workers --- End diff -- Actually, I see the display name is different. Doh. But here we are using the terms "Elasticsearch" and "HDFS", rather than "Random Access" and "Batch". We should probably stick with one nomenclature. ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r150314839
--- Diff:
metron-platform/metron-indexing/src/main/scripts/start_hdfs_topology.sh ---
@@ -0,0 +1,22 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+METRON_VERSION=${project.version}
+METRON_HOME=/usr/metron/$METRON_VERSION
+TOPOLOGY_JAR=metron-elasticsearch-$METRON_VERSION-uber.jar
+storm jar $METRON_HOME/lib/$TOPOLOGY_JAR org.apache.storm.flux.Flux
--remote $METRON_HOME/flux/indexing/batch/remote.yaml --filter
$METRON_HOME/config/hdfs.properties
--- End diff --
In the MPack we refer to this as the "batch" topology. But in this script
we refer to it as the "HDFS" topology. We should probably stick with one
nomenclature.
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user nickwallen commented on a diff in the pull request: https://github.com/apache/metron/pull/831#discussion_r150314308 --- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/configuration/metron-indexing-env.xml --- @@ -73,18 +98,31 @@ Indexing Update Column Family -indexing_workers +ra_indexing_workers +Number of Indexing Topology Workers --- End diff -- With the name and description the same for the random access and batch topologies; can the user distinguish which is which? ---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
Github user nickwallen commented on a diff in the pull request:
https://github.com/apache/metron/pull/831#discussion_r150314117
--- Diff:
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/indexing_commands.py
---
@@ -162,34 +164,77 @@ def start_indexing_topology(self, env):
self.__params.metron_principal_name,
execute_user=self.__params.metron_user)
-start_cmd_template =
"""{0}/bin/start_elasticsearch_topology.sh \
--s {1} \
--z {2}"""
-start_cmd =
start_cmd_template.format(self.__params.metron_home,
- self.__indexing_topology,
-
self.__params.zookeeper_quorum)
+start_cmd_template = """{0}/bin/start_hdfs_topology.sh"""
+start_cmd =
start_cmd_template.format(self.__params.metron_home)
Execute(start_cmd, user=self.__params.metron_user, tries=3,
try_sleep=5, logoutput=True)
else:
-Logger.info('Indexing topology already running')
+Logger.info('Batch Indexing topology already running')
-Logger.info('Finished starting indexing topology')
+Logger.info('Finished starting batch indexing topology')
-def stop_indexing_topology(self, env):
-Logger.info('Stopping ' + self.__indexing_topology)
+def start_random_access_indexing_topology(self, env):
+Logger.info('Starting ' + self.__random_access_indexing_topology)
--- End diff --
First off, I think we definitely need to make this happen. Each index
destination is going to have very different performance characteristics that
need to be tuned in isolation. I think this is a step in the right direction.
As I read this we have effectively hard-coded two indexing topologies;
random access and batch. This is definitely the most logical way to get to
separate topologies based on our existing code base. But I am wondering if we
might think about this in a slightly different way.
What I really like about indexing is that we have the idea of multiple,
independent destinations. For example, my indexing configuration could look
like this.
```
{
"elasticsearch": {
"index": "foo",
"enabled" : true
},
"hdfs": {
"index": "foo",
"batchSize": 1,
"enabled" : true
}
}
```
What if we introduced logic that consumes the indexing configuration,
determines that it needs to launch 2 topologies in this case, and then launches
those 2 separate topologies? If I had 3 destinations configured, then it would
launch 3 topologies; one for each destination?
I can definitely see the extra complexity in doing this. You have to make
sure the user can independently configure each of the topologies. You have to
respond to configuration changes made by the user. And probably a few other
complications.
But these are already complications that we need to deal with in Parsing.
A user can define 1 to N Parsing topologies. It seems like if we can solve
these challenges for Parsing, we can do the same for Indexing.
Anywho, I can totally see this PR as a near-term solution to the immediate
problem, which might lead towards a longer-term solution like I propose. I
just wanted to see if anyone had related thoughts.
---
[GitHub] metron pull request #831: METRON-1302: Split up Indexing Topology into batch...
GitHub user cestella opened a pull request: https://github.com/apache/metron/pull/831 METRON-1302: Split up Indexing Topology into batch and random access sections ## Contributor Comments Currently we have the indexing topology handle writing to both random access indices (e.g. elasticsearch) as well as batch write indices (e.g. hdfs). We should split these up and configure them separately. In terms of manual testing, full-dev should continue to function in that: * Elasticsearch should have data written to it * HDFS should have data written to it * The indexing configuration should have separate subsections for random access and batch index writers ## Pull Request Checklist Thank you for submitting a contribution to Apache Metron. Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions. Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides. In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel). - [x] Does your PR title start with METRON- where is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. - [x] Has your PR been rebased against the latest commit within the target branch (typically master)? ### For code changes: - [x] Have you included steps to reproduce the behavior or problem that is being changed or addressed? - [x] Have you included steps or a guide to how the change may be verified and tested manually? - [x] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via: ``` mvn -q clean integration-test install && build_utils/verify_licenses.sh ``` - [x] Have you written or updated unit tests and or integration tests to verify your changes? - [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? - [x] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent? ### For documentation related changes: - [x] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`: ``` cd site-book mvn site ``` Note: Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request. You can merge this pull request into a Git repository by running: $ git pull https://github.com/cestella/incubator-metron indexing_topo_split Alternatively you can review and apply these changes as the patch at: https://github.com/apache/metron/pull/831.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 #831 ---
