You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by merrimanr <gi...@git.apache.org> on 2017/11/28 22:11:05 UTC
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
GitHub user merrimanr opened a pull request:
https://github.com/apache/metron/pull/853
METRON-1337: List of facets should not be hardcoded
## Contributor Comments
This PR makes the list of facet fields in the Alerts UI configurable. There is now a "search.facet.fields" setting in https://github.com/apache/metron/blob/master/metron-interface/metron-rest/src/main/resources/application.yml that is a comma-separate list of fields to be used as facets. Originally a comment was made that the "host" field wasn't commonly used so I removed that from the default list.
I can think of two options for exposing this configuration:
- through an AlertProfile that is specific to each user, stored in a relational database and can be edited through REST
- a setting that is exposed in Ambari
I chose to include the first option in this PR to get the conversation going. Is one of these preferable? The AlertProfile approach allows this setting to be changed at runtime and each user has their own list of facet fields. But it is not versioned like it would if it were in Ambari. Do we prefer one over the other? Do we want both with Ambari being the default when an AlertProfile doesn't exist for a user? Are there other options I'm not thinking of?
This works similar to how default search indices work: it is managed in the REST layer and can be overriden by including facet fields in a search request. However it seemed useful to allow a way to explicitly NOT include facets in a query so it works slightly different than indices. A missing facetFields property in the request will use the defaults while an empty array will disable facets. A missing indices property or an empty array will use the default indices. Is this the correct behavior?
This has been tested in full dev and the UI e2e tests pass when run in isolation. There is currently an effort to stabilize the e2e tests as a follow on to https://github.com/apache/metron/pull/803 so I did not try to solve that here.
I will add some documentation around configuring the facet field list and facetFields behavior in a search request once we come to a consensus on the solution.
Another issue I would like to point out. When I added facetFields to the AlertProfile object it required a database update because that new field needed to be added to the database. This would become an issue if someone were upgrading from a previous version. Is this acceptable if we document it for future upgrades? Is a relational database the right solution or should we consider a more flexible storage option?
## 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-XXXX where XXXX 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/merrimanr/incubator-metron configurable-facets
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/853.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 #853
----
commit 8612bc286cd153e1358e405d78516d4bc004201b
Author: merrimanr <me...@gmail.com>
Date: 2017-11-28T16:51:09Z
initial commit
commit 0084af4e29e825d589231cae208f199a75a6c308
Author: merrimanr <me...@gmail.com>
Date: 2017-11-28T16:51:28Z
Merge remote-tracking branch 'mirror/master' into configurable-facets
commit e6fb5ed879cc690d34482224e4fd353d82ecfdc1
Author: merrimanr <me...@gmail.com>
Date: 2017-11-28T21:35:28Z
updated e2e tests
----
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
Done
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
Ambari does not manage this config. It is only included in the base application.yml as a default setting. I don't feel like this setting should be in Ambari for a couple reasons: changing it requires a restart and the list of facets cannot be user specific.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
Any other feedback @ottobackwards or is this ready to go?
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/853
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
I think it's a fair question @ottobackwards. Anything that might affect how the HBase table is laid out should be worked out now or we're back to altering tables during upgrades. Currently the row key is the user name, the column family is hardcoded (although configurable), and the column qualifier is hardcoded. Would you add the type to the row key or store different client settings in different columns?
We could add versioning but that is not trivial and will increase the scope of this PR. I can take it on here if we decide we want it but this PR is already large. Follow on maybe? Either way works for me. If we decide a follow on is better I would make an effort to do it right away before we do a release.
Jackson is the standard in Metron for serialization/deserialization. I think we should have a very compelling reason to introduce something new. Happy to discuss.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
This looks great, one small comment from review
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
First, nice work.
Second, I have this question:
Since we will have some unknown number of rest clients, that may want to save 'configuration' associated with a user, and not just Alerts UI, should it not be factored more generically?
getUserSettingsForType()
And stored as user, type, settings byte[] or some such?
We may also want to store the version so that it is user, type, settingsFmtVersion, settings.
We might also want to consider storing as Protobufs, which are more supporting changes.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/853
Bump @merrimanr
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r168816712
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/search/SearchRequest.java ---
@@ -101,16 +99,16 @@ public void setSort(List<SortField> sort) {
this.sort = sort;
}
- public Optional<List<String>> getFields() {
- return fields == null || fields.size() == 0 ? Optional.empty() : Optional.of(fields);
+ public List<String> getFields() {
+ return fields;
}
public void setFields(List<String> fields) {
this.fields = fields;
}
- public Optional<List<String>> getFacetFields() {
- return facetFields == null || facetFields.size() == 0 ? Optional.empty() : Optional.of(facetFields);
+ public List<String> getFacetFields() {
+ return facetFields;
--- End diff --
Because null and an empty array mean 2 different things. See point 6 here: https://github.com/apache/metron/pull/853#issuecomment-356350839.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/853
@merrimanr this looks much better. I'm +1 via inspection pending any further feedback from @ottobackwards.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
I am not an expert in hbase, I cannot say how i'd implement it.
The version thing we can leave out, it will be just as good or bad as what we have in zookeeper right?
I think the use case is:
For any given User ( at least ) we need the ability to store currently known and future unknown configurations as bytes, with configuration type identifier lookup. This should be done such that new configuration types can be added by 'putting' config bytes with a new identifier.
I think that gets us a long way.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
What I get to was, the setting should be exposed in ambari if ambari manages the config.
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r168817020
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/config/HBaseConfig.java ---
@@ -0,0 +1,61 @@
+/**
+ * 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.
+ */
+package org.apache.metron.rest.config;
+
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.client.HTableInterface;
+import org.apache.metron.hbase.HTableProvider;
+import org.apache.metron.rest.MetronRestConstants;
+import org.apache.metron.rest.RestException;
+import org.apache.metron.rest.service.GlobalConfigService;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Profile;
+import org.springframework.core.env.Environment;
+
+import java.io.IOException;
+
+import static org.apache.metron.rest.MetronRestConstants.TEST_PROFILE;
+import static org.apache.metron.rest.repository.UserSettingsRepository.USER_SETTINGS_HBASE_TABLE;
+
+@Configuration
+@Profile("!" + TEST_PROFILE)
--- End diff --
We don't want to use this Configuration if tests are running. There is a separate TestConfig that setups up mock, inmemory components, etc.
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r169455383
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/AlertServiceImpl.java ---
@@ -37,15 +47,21 @@
@Service
public class AlertServiceImpl implements AlertService {
--- End diff --
Sure I can do that. Since these settings are specific to the Alerts UI, should we include "ui" too? Should the REST service also follow a similar pattern?
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
I will address 6 shortly. For 5, should we explore a more flexible store in this PR? Or at least validate that an RDBMS is the right choice? I think this is something we should tackle now as it will likely affect future work as well. Does it make sense to start a discuss thread on this topic?
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
Does ambari own the application.yaml currently?
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/853
We're good on 1-4.
For point 5. I'd still like to see a note in the upgrading, even if it gets removed when the more flexible store is added. I don't know when we'll get around to using a different store, and if it ends up not being this release, we'll need to add this anyway.
I think point 6 still needs to be addressed.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
I took @ottobackwards's suggestion and added a "type" parameter that can be used to independently manage user settings for different types of clients. I moved the users settings for the Alerts UI (the original intention of this PR) back to the AlertController. I also went ahead and moved the low level client code to the metron-hbase module so that this user settings abstraction can be used anywhere in Metron. For examples of how to setup and use the client see HBaseConfig and AlertServiceImpl in the metron-rest module.
Testing in full dev is slightly different now. Instead of the endpoints being in the UserController they should now be tested in the AlertController. The Alerts UI should also be tested. Navigate to the Alerts UI and you should see the default facets on the left. Update the "facetFields" user setting with Swagger and the new list should appear in the Alerts UI after refreshing. For example, if you set facetFields = ip_src_addr, only that facet field should appear in the Alerts UI.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/853
Just ducking in here, @merrimanr is this ready for review? Specifically, the responses to @justinleet 's questions have all been factored into the current code for this PR, right? If yes, @justinleet would you agree or is there more work to be done to react to your comments?
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r171015228
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/search/SearchRequest.java ---
@@ -101,16 +99,16 @@ public void setSort(List<SortField> sort) {
this.sort = sort;
}
- public Optional<List<String>> getFields() {
- return fields == null || fields.size() == 0 ? Optional.empty() : Optional.of(fields);
+ public List<String> getFields() {
+ return fields;
}
public void setFields(List<String> fields) {
this.fields = fields;
}
- public Optional<List<String>> getFacetFields() {
- return facetFields == null || facetFields.size() == 0 ? Optional.empty() : Optional.of(facetFields);
+ public List<String> getFacetFields() {
+ return facetFields;
--- End diff --
Ok, sounds reasonable
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
Here are my thoughts on your responses.
1. I think we're in agreement. This is how this PR currently works.
2. No, saved searches are currently stored client-side. I think the plan was to store these server-side but it hasn't been implemented yet. The only thing we store in a relational DB at this point are REST user credentials. This would change if we switch to an LDAP approach for managing authentication (I think we should) and nothing we be stored in a DB at that point.
3. Agreed.
4. Agreed. I don't see much value in versioning a user's displayable facet fields.
5. My point here is that a more flexible store for things like this (user profile settings) may eliminate this upgrade complexity altogether.
6. I'm good with that.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
Of course re-reading your comment, I see you specifically didn't account for these, but I think there are some basic tweaks to allow for this that don't presuppose far flung 20% use cases.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
Each environment will have it's own application.yaml. Full dev has one, our testing environment has one, Ambari ships one etc. There is also a base application.yaml that has the defaults (default facets are included here). Does that make sense?
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r169515112
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/AlertServiceImpl.java ---
@@ -37,15 +47,21 @@
@Service
public class AlertServiceImpl implements AlertService {
--- End diff --
That seems reasonable
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/853
@merrimanr Can you deconflict this?
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r170120909
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/AlertServiceImpl.java ---
@@ -37,15 +47,21 @@
@Service
public class AlertServiceImpl implements AlertService {
--- End diff --
Name is changed in the latest commit. Let me know what you think.
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r170125479
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/AlertServiceImpl.java ---
@@ -37,15 +47,21 @@
@Service
public class AlertServiceImpl implements AlertService {
--- End diff --
I think that is great. At some point, if we are going to do this a few times we should have a standard naming convention ( separators at least or something). But that isn't a thing for this PR
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/853
The latest commit switches the persistence for storing user settings to HBase rather than a RDBMS as discussed on the dev list. Instead of fields being stored in RDBMS columns, the user settings object is now serialized with Jackson and stored in HBase as a byte[]. This required several changes including:
- Added code to setup HBase in the MPack, following the update table pattern in the indexing MPack scripts as an example
- Added default table/cf values to REST properties
- Added a Config class to the REST app to setup an HBase client
- Added a Service class to the REST app that manages user settings in HBase
- Added tests for all new classes and updated existing tests
- Updated the REST README
I also refactored some areas to make things clearer and easier to understand:
- Renamed AlertProfile to UserSettings and moved the endpoints to the UserController. The thinking is that we may not want this to be limited to just the Alerts UI.
- Refactored the REST MPack scripts to more closely align with other components in regards to setting up Kafka, HBase, etc.
- Removed the Optional fields from a couple model classes. This was causing issues with Jackson and I don't believe it benefits us enough to have to deal with that complexity.
There are a couple of design issues to consider when reviewing this. The implementation in the REST app is specific to this use case. I considered trying to make it more generic for future HBase use cases but decided to keep it simple for now. Instead of trying to predict what those use cases look like and choose a pattern that works, I decided to leave that to whoever implements a new use case in the future. I also considered making this generic to all of Metron but again, decided to keep it simple. Should these settings be limited to just REST/UIs? Any thoughts on this?
This has been tested in full dev and all tests are passing. In full dev, navigate to the UserController and use the various endpoints to save and retrieve settings for the currently logged in user. You should also be able to log in as the admin user and see all user settings and delete an individual user's settings. I also tested evolving the user settings model by adding new fields and it worked without issue. Users with existing settings just return null for new fields.
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r169441455
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/AlertServiceImpl.java ---
@@ -37,15 +47,21 @@
@Service
public class AlertServiceImpl implements AlertService {
--- End diff --
can we make the type metron_alert? alert seems a little generic
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r168816212
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/rest_commands.py ---
@@ -179,6 +203,35 @@ def status_rest_application(self, env):
self.__params.metron_rest_port,
self.__params.metron_user)
+ def create_hbase_tables(self):
+ Logger.info("Creating HBase Tables")
+ metron_service.create_hbase_table(self.__params,
+ self.__params.user_settings_hbase_table,
+ self.__params.user_settings_hbase_cf)
+ Logger.info("Done creating HBase Tables")
+ self.set_hbase_configured()
+
+ def set_hbase_acls(self):
+ Logger.info("Setting HBase ACLs")
+ if self.__params.security_enabled:
+ kinit(self.__params.kinit_path_local,
+ self.__params.hbase_keytab_path,
+ self.__params.hbase_principal_name,
+ execute_user=self.__params.hbase_user)
+
+ cmd = "echo \"grant '{0}', 'RW', '{1}'\" | hbase shell -n"
--- End diff --
Good catch.
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r168809125
--- Diff: metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/search/SearchRequest.java ---
@@ -101,16 +99,16 @@ public void setSort(List<SortField> sort) {
this.sort = sort;
}
- public Optional<List<String>> getFields() {
- return fields == null || fields.size() == 0 ? Optional.empty() : Optional.of(fields);
+ public List<String> getFields() {
+ return fields;
}
public void setFields(List<String> fields) {
this.fields = fields;
}
- public Optional<List<String>> getFacetFields() {
- return facetFields == null || facetFields.size() == 0 ? Optional.empty() : Optional.of(facetFields);
+ public List<String> getFacetFields() {
+ return facetFields;
--- End diff --
If we're going to change the interface, what about
```
return facetFields == null ? new ArrayList() : facetFields;
```
so it's not null.
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r168810966
--- Diff: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/config/HBaseConfig.java ---
@@ -0,0 +1,61 @@
+/**
+ * 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.
+ */
+package org.apache.metron.rest.config;
+
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.client.HTableInterface;
+import org.apache.metron.hbase.HTableProvider;
+import org.apache.metron.rest.MetronRestConstants;
+import org.apache.metron.rest.RestException;
+import org.apache.metron.rest.service.GlobalConfigService;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Profile;
+import org.springframework.core.env.Environment;
+
+import java.io.IOException;
+
+import static org.apache.metron.rest.MetronRestConstants.TEST_PROFILE;
+import static org.apache.metron.rest.repository.UserSettingsRepository.USER_SETTINGS_HBASE_TABLE;
+
+@Configuration
+@Profile("!" + TEST_PROFILE)
--- End diff --
Test profile?
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/853
I don't see anything that should be problematic in ES 5.6.2, but can you confirm @merrimanr?
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
+1 by inspection
---
[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/853#discussion_r168802913
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/rest_commands.py ---
@@ -179,6 +203,35 @@ def status_rest_application(self, env):
self.__params.metron_rest_port,
self.__params.metron_user)
+ def create_hbase_tables(self):
+ Logger.info("Creating HBase Tables")
+ metron_service.create_hbase_table(self.__params,
+ self.__params.user_settings_hbase_table,
+ self.__params.user_settings_hbase_cf)
+ Logger.info("Done creating HBase Tables")
+ self.set_hbase_configured()
+
+ def set_hbase_acls(self):
+ Logger.info("Setting HBase ACLs")
+ if self.__params.security_enabled:
+ kinit(self.__params.kinit_path_local,
+ self.__params.hbase_keytab_path,
+ self.__params.hbase_principal_name,
+ execute_user=self.__params.hbase_user)
+
+ cmd = "echo \"grant '{0}', 'RW', '{1}'\" | hbase shell -n"
--- End diff --
Do we check that the REST API has the hbase client also installed on the node? This would be needed for this to work for > 1 nodes.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/853
To respond to the questions in the description (and maybe kick off conversation, especially if anyone disagrees) + add my own thoughts. In no particular order.
@merrimanr Let me know if I'm misunderstanding anything as well, I'm digging into this for the first time
1. This feels like a user specific config. Having a default that each user gets if they don't do anything seems appropriate, but I'd expect different people to want to be looking at different things.
1. The DB feels like a reasonable place for this to live. We're already storing some per user type data (e.g. recent searches) in the DB, right?
1. It would be nice to have (and this would be follow-on), a really easy UI way to adjust the facet fields. Especially, if it's that adjustable on the fly, I don't think we need versioning on the user level facet configs.
1. IMO, having the defaults be versioned is nice, but not mandatory (we don't version anything in that file, do we?)
1. Re: the upgrade issue. We generally don't support upgrades, but having the upgrading.md have a note along with at least a couple of the basic commands to add that field would be nice.
1. I'd have a slight inclination towards empty facet array being default and missing being disable. It's more intuitive for me that if I leave it out of the request, it's not present.
---
[GitHub] metron issue #853: METRON-1337: List of facets should not be hardcoded
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/853
Sorry, I'll try to get back to this today
---