You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/01/24 19:57:32 UTC
[GitHub] [incubator-pinot] galalen opened a new pull request #6485: [WIP] add schema validation mechanism
galalen opened a new pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485
## Description
Configurable JSON schema validation for table config, solve [issue 6300](https://github.com/apache/incubator-pinot/issues/6300)
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] galalen commented on a change in pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
galalen commented on a change in pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#discussion_r570289964
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/PinotJsonSchemasValidator.java
##########
@@ -0,0 +1,16 @@
+package org.apache.pinot.spi.utils.validator;
+
+public class PinotJsonSchemasValidator extends BaseJsonValidator {
+
+ private static final String BASE_SCHEMA_DIR = "/schemas";
+
+ private static final String TABLE_CONFIG_SCHEMA = BASE_SCHEMA_DIR + "/tableConfig.json";
Review comment:
For now, we don't but, in case we want to add schema validation to another thing.
But I'll remove it anyway
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#discussion_r588042899
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/PinotConfigValidator.java
##########
@@ -0,0 +1,33 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface PinotConfigValidator extends Serializable {
Review comment:
javadoc please
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/BasePinotConfigValidator.java
##########
@@ -0,0 +1,80 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.LogLevel;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+/**
+* Base implementation for validating JSON schemas
+* */
+abstract public class BasePinotConfigValidator implements PinotConfigValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ /**
+ * Load schema from project resources
+ * */
+ protected void loadSchema(String path)
+ throws IOException, ProcessingException {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
Review comment:
Does this need to be inside the method or can be a class member variable ?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/BasePinotConfigValidator.java
##########
@@ -0,0 +1,80 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.LogLevel;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+/**
+* Base implementation for validating JSON schemas
+* */
+abstract public class BasePinotConfigValidator implements PinotConfigValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ /**
+ * Load schema from project resources
+ * */
+ protected void loadSchema(String path)
+ throws IOException, ProcessingException {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ }
+
+ /**
+ * Validate passed jsonStr schema against the loaded schema from resources
+ * */
+ @Override
+ public boolean validate(String jsonStr)
+ throws ProcessingException {
+ JsonNode node = null;
+ try {
+ node = JsonUtils.stringToJsonNode(jsonStr);
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
+ _report = _schema.validate(node);
+ return _report.isSuccess();
+ }
+
+ /**
+ * Report validation messages if validation fail
+ * */
+ @Override
+ public List<String> getValidationMessages() {
Review comment:
This method assumes the caller has already invoked validate(jsonStr) : see my comment below in the interface class regarding simplifying this dependency
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/PinotConfigValidator.java
##########
@@ -0,0 +1,33 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface PinotConfigValidator extends Serializable {
+
+ boolean validate(String jsonStr)
+ throws ProcessingException;
+
+ List<String> getValidationMessages();
Review comment:
I wonder if we should have just one method - that takes a jsonStr and returns a list of validation messages. That way there's no dependency on the caller side to invoke validate first and then invoke getValidationMessages.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/PinotConfigSchemasValidator.java
##########
@@ -0,0 +1,44 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import java.io.IOException;
+
+/**
+ * Factory to help create validator for [Table Config,]
+ * */
+public class PinotConfigSchemasValidator extends BasePinotConfigValidator {
Review comment:
Rename to PinotTableConfigSchemaValidator ?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/BasePinotConfigValidator.java
##########
@@ -0,0 +1,80 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.LogLevel;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+/**
+* Base implementation for validating JSON schemas
+* */
+abstract public class BasePinotConfigValidator implements PinotConfigValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ /**
+ * Load schema from project resources
+ * */
+ protected void loadSchema(String path)
+ throws IOException, ProcessingException {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ }
+
+ /**
+ * Validate passed jsonStr schema against the loaded schema from resources
+ * */
+ @Override
+ public boolean validate(String jsonStr)
+ throws ProcessingException {
+ JsonNode node = null;
+ try {
+ node = JsonUtils.stringToJsonNode(jsonStr);
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
Should we throw if we can't get a valid Json ?
##########
File path: pinot-core/src/test/java/org/apache/pinot/util/validator/PinotConfigSchemasValidatorTest.java
##########
@@ -0,0 +1,9 @@
+package org.apache.pinot.util.validator;
+
+import org.testng.annotations.Test;
+
+
+public class PinotConfigSchemasValidatorTest {
Review comment:
Empty ?
##########
File path: pinot-core/src/test/java/org/apache/pinot/util/validator/BasePinotConfigValidatorTest.java
##########
@@ -0,0 +1,62 @@
+package org.apache.pinot.util.validator;
+
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import org.apache.pinot.core.util.validator.BasePinotConfigValidator;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class BasePinotConfigValidatorTest extends BasePinotConfigValidator {
+
+ @Test
+ public void testLoadingSchema()
+ throws IOException, ProcessingException {
+ // _schema object is null currently
+ Assert.assertNull(_schema);
+
+ loadSchema("/schemas/tableConfig.json");
+ // _schema is store a valid schema after loading
+ Assert.assertNotNull(_schema);
+
+ // fail and throw IOException if schema not found
+ try {
+ loadSchema("/no/schema/here");
+ Assert.fail("schema not found");
+ } catch (IOException e) {
+ // expected
+ }
+ }
+
+ @Test
+ public void testValidateSchema()
+ throws IOException, ProcessingException {
+
+ // given a valid schema, validate should returns true with not error messages
+ String airlinesTableConfig = readSampleJson("/schemas/good_airlines_table_config.json");
+ loadSchema("/schemas/tableConfig.json");
+ boolean result = validate(airlinesTableConfig);
+ Assert.assertEquals(getValidationMessages().size(), 0);
+ Assert.assertTrue(result);
+
+ // given a invalid schema [invalid key is table_type], validate should returns false
+ airlinesTableConfig = readSampleJson("/schemas/bad_airlines_table_config.json");
+ loadSchema("/schemas/tableConfig.json");
+ result = validate(airlinesTableConfig);
+ Assert.assertEquals(getValidationMessages().size(), 1);
+ Assert.assertFalse(result);
+ }
+
+ private String readSampleJson(String path)
Review comment:
Do we need this ? I feel some Json Utils method might do the trick here ?
##########
File path: pinot-core/src/test/java/org/apache/pinot/util/validator/BasePinotConfigValidatorTest.java
##########
@@ -0,0 +1,62 @@
+package org.apache.pinot.util.validator;
+
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import org.apache.pinot.core.util.validator.BasePinotConfigValidator;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class BasePinotConfigValidatorTest extends BasePinotConfigValidator {
+
+ @Test
+ public void testLoadingSchema()
+ throws IOException, ProcessingException {
+ // _schema object is null currently
+ Assert.assertNull(_schema);
+
+ loadSchema("/schemas/tableConfig.json");
+ // _schema is store a valid schema after loading
+ Assert.assertNotNull(_schema);
+
+ // fail and throw IOException if schema not found
+ try {
+ loadSchema("/no/schema/here");
+ Assert.fail("schema not found");
Review comment:
nit: rename error message to : "should fail for missing schema"
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/validator/PinotConfigSchemasValidator.java
##########
@@ -0,0 +1,44 @@
+/**
+ * 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.pinot.core.util.validator;
+
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import java.io.IOException;
+
+/**
+ * Factory to help create validator for [Table Config,]
+ * */
+public class PinotConfigSchemasValidator extends BasePinotConfigValidator {
+
+
+ private static final String TABLE_CONFIG_SCHEMA = "/schemas/tableConfig.json";
+
+ private PinotConfigSchemasValidator(String schema)
+ throws IOException, ProcessingException {
+ loadSchema(schema);
+ }
+
+ /**
+ * Create validator for table config
+ * */
+ public static PinotConfigSchemasValidator forTableConfig()
Review comment:
I think we should make this a singleton.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#discussion_r568227370
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ protected void loadSchema(String path) {
+ try {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ } catch (IOException | ProcessingException e) {
+ e.printStackTrace();
Review comment:
Avoid using printStackTrace, you can use a logger or simply throw the exception to the caller. Same comment for all such instances
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ protected void loadSchema(String path) {
+ try {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ } catch (IOException | ProcessingException e) {
+ e.printStackTrace();
+ }
+ }
+
+ @Override
+ public boolean validate(String jsonStr) {
+ JsonNode node = null;
+ try {
+ node = JsonUtils.stringToJsonNode(jsonStr);
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
Please handle errors correctly - I guess in this case - we should throw ?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/PinotJsonSchemasValidator.java
##########
@@ -0,0 +1,16 @@
+package org.apache.pinot.spi.utils.validator;
+
+public class PinotJsonSchemasValidator extends BaseJsonValidator {
+
+ private static final String BASE_SCHEMA_DIR = "/schemas";
+
+ private static final String TABLE_CONFIG_SCHEMA = BASE_SCHEMA_DIR + "/tableConfig.json";
Review comment:
Do we need 2 separate constants for this ?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
Review comment:
nit: please add javadoc
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/JsonValidator.java
##########
@@ -0,0 +1,13 @@
+package org.apache.pinot.spi.utils.validator;
+
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface JsonValidator extends Serializable {
Review comment:
Needs javadoc
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/JsonValidator.java
##########
@@ -0,0 +1,13 @@
+package org.apache.pinot.spi.utils.validator;
+
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface JsonValidator extends Serializable {
Review comment:
Maybe use a better name for this interface ? Do we expect this to be a common interface for any kind of json ? Or just limited to config (table or schema) ?
If its just config - we could also say PinotConfigValidator or something ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#issuecomment-766430863
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=h1) Report
> Merging [#6485](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=desc) (f90c8ea) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.64%`.
> The diff coverage is `42.04%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6485 +/- ##
===========================================
- Coverage 66.44% 43.80% -22.65%
===========================================
Files 1075 1336 +261
Lines 54773 65611 +10838
Branches 8168 9566 +1398
===========================================
- Hits 36396 28741 -7655
- Misses 15700 34464 +18764
+ Partials 2677 2406 -271
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.80% <42.04%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1330 more](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=footer). Last update [830eb94...f90c8ea](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#issuecomment-766430863
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=h1) Report
> Merging [#6485](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=desc) (f90c8ea) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.64%`.
> The diff coverage is `42.04%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6485 +/- ##
===========================================
- Coverage 66.44% 43.80% -22.65%
===========================================
Files 1075 1336 +261
Lines 54773 65611 +10838
Branches 8168 9566 +1398
===========================================
- Hits 36396 28741 -7655
- Misses 15700 34464 +18764
+ Partials 2677 2406 -271
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.80% <42.04%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1330 more](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=footer). Last update [830eb94...f90c8ea](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] galalen commented on a change in pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
galalen commented on a change in pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#discussion_r570289964
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/PinotJsonSchemasValidator.java
##########
@@ -0,0 +1,16 @@
+package org.apache.pinot.spi.utils.validator;
+
+public class PinotJsonSchemasValidator extends BaseJsonValidator {
+
+ private static final String BASE_SCHEMA_DIR = "/schemas";
+
+ private static final String TABLE_CONFIG_SCHEMA = BASE_SCHEMA_DIR + "/tableConfig.json";
Review comment:
For now, we don't but, in case we want to add schema validation to another thing.
But I'll remove it anyway
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#discussion_r568227370
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ protected void loadSchema(String path) {
+ try {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ } catch (IOException | ProcessingException e) {
+ e.printStackTrace();
Review comment:
Avoid using printStackTrace, you can use a logger or simply throw the exception to the caller. Same comment for all such instances
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ protected void loadSchema(String path) {
+ try {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ } catch (IOException | ProcessingException e) {
+ e.printStackTrace();
+ }
+ }
+
+ @Override
+ public boolean validate(String jsonStr) {
+ JsonNode node = null;
+ try {
+ node = JsonUtils.stringToJsonNode(jsonStr);
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
Please handle errors correctly - I guess in this case - we should throw ?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/PinotJsonSchemasValidator.java
##########
@@ -0,0 +1,16 @@
+package org.apache.pinot.spi.utils.validator;
+
+public class PinotJsonSchemasValidator extends BaseJsonValidator {
+
+ private static final String BASE_SCHEMA_DIR = "/schemas";
+
+ private static final String TABLE_CONFIG_SCHEMA = BASE_SCHEMA_DIR + "/tableConfig.json";
Review comment:
Do we need 2 separate constants for this ?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
Review comment:
nit: please add javadoc
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/JsonValidator.java
##########
@@ -0,0 +1,13 @@
+package org.apache.pinot.spi.utils.validator;
+
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface JsonValidator extends Serializable {
Review comment:
Needs javadoc
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/JsonValidator.java
##########
@@ -0,0 +1,13 @@
+package org.apache.pinot.spi.utils.validator;
+
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface JsonValidator extends Serializable {
Review comment:
Maybe use a better name for this interface ? Do we expect this to be a common interface for any kind of json ? Or just limited to config (table or schema) ?
If its just config - we could also say PinotConfigValidator or something ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6485: [WIP] add schema validation mechanism for table config
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#issuecomment-766430863
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=h1) Report
> Merging [#6485](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=desc) (59d77d8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.55%`.
> The diff coverage is `42.85%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6485 +/- ##
===========================================
- Coverage 66.44% 43.89% -22.56%
===========================================
Files 1075 1361 +286
Lines 54773 66548 +11775
Branches 8168 9703 +1535
===========================================
- Hits 36396 29213 -7183
- Misses 15700 34895 +19195
+ Partials 2677 2440 -237
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.89% <42.85%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1387 more](https://codecov.io/gh/apache/incubator-pinot/pull/6485/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=footer). Last update [5137025...59d77d8](https://codecov.io/gh/apache/incubator-pinot/pull/6485?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org