Indicate what fields are getting ignored when adding configs #8514

saurabhd336 · 2022-04-12T16:56:46Z

This PR adds "unrecognizedProperties" to POST /tables/, PUT /tables/{tableName} and POST /tables/validate. This field will be a flattened map of json keys to values that will let the API users know when certain field(s) in the input config json are being ignored.

saurabhd336 · 2022-04-12T16:57:11Z

GH Issue: #8318

codecov-commenter · 2022-04-13T10:55:38Z

Codecov Report

Merging #8514 (00d6277) into master (b0144cf) will decrease coverage by 1.04%.
The diff coverage is 84.44%.

@@             Coverage Diff              @@
##             master    #8514      +/-   ##
============================================
- Coverage     70.65%   69.61%   -1.05%     
+ Complexity     4314     4313       -1     
============================================
  Files          1692     1693       +1     
  Lines         88751    88786      +35     
  Branches      13468    13470       +2     
============================================
- Hits          62708    61806     -902     
- Misses        21657    22611     +954     
+ Partials       4386     4369      -17

Flag	Coverage Δ
integration1	`27.31% <35.55%> (+0.07%)`	⬆️
integration2	`?`
unittests1	`66.93% <68.18%> (+<0.01%)`	⬆️
unittests2	`14.17% <51.11%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...inot/controller/api/resources/SuccessResponse.java	`100.00% <ø> (ø)`
...ain/java/org/apache/pinot/spi/utils/JsonUtils.java	`67.06% <68.18%> (+0.10%)`	⬆️
...ontroller/api/resources/ConfigSuccessResponse.java	`100.00% <100.00%> (ø)`
...oller/api/resources/PinotTableRestletResource.java	`50.67% <100.00%> (+0.49%)`	⬆️
...apache/pinot/common/helix/ExtraInstanceConfig.java	`0.00% <0.00%> (-100.00%)`	⬇️
...t/core/plan/StreamingInstanceResponsePlanNode.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ore/operator/streaming/StreamingResponseUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ager/realtime/PeerSchemeSplitSegmentCommitter.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ator/streaming/StreamingSelectionOnlyOperator.java	`0.00% <0.00%> (-90.00%)`	⬇️
...he/pinot/core/plan/StreamingSelectionPlanNode.java	`0.00% <0.00%> (-88.89%)`	⬇️
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89836a5...00d6277. Read the comment docs.

Jackie-Jiang

This is awesome! I can see it improve user experience drastically!

Jackie-Jiang · 2022-04-21T17:55:31Z

...ontroller/src/main/java/org/apache/pinot/controller/api/resources/ConfigSuccessResponse.java

+import java.util.Map;
+
+public final class ConfigSuccessResponse extends SuccessResponse {
+  private final Map<String, Object> _unparseableProps;


Suggest renaming the field, same for other places

Suggested change

private final Map<String, Object> _unparseableProps;

private final Map<String, Object> _ unrecognizedProperties;

Jackie-Jiang · 2022-04-21T17:58:46Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java


  public static <T> T stringToObject(String jsonString, Class<T> valueType)
      throws IOException {
    return DEFAULT_READER.forType(valueType).readValue(jsonString);
  }

+  public static <T> JsonPojoWithUnparsableProps<T> stringToObjectAndUnparseableProps(String jsonString, Class<T> klass)


Name the second parameter valueType for consistency

(optional) I feel returning a Pair<T, Map<String, Object>> should be enough

Ack on https://github.com/apache/pinot/pull/8514/files#r855450400
#2 The Pair class defined within the project expects 'T' to be extending Serializable, which would mean changing the base response classes. Other Pair implementations would need dependencies. Leaving it as it is for now.. Will revisit.

Can we use org.apache.commons.lang3.tuple.Pair? I think we should already have the dependency

Jackie-Jiang · 2022-04-21T18:02:55Z

pinot-spi/src/test/java/org/apache/pinot/spi/utils/TestClass.java

+  int _primitiveIntegerField;
+  String _stringField;
+  Long _longField;
+  Klass _classField;


(minor) I think we use Clazz in most of the places

Jackie-Jiang · 2022-04-21T18:18:19Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java

+  }
+
+  private static Stream<Map.Entry<String, Object>> flatten(Map.Entry<String, Object> entry) {
+


(nit) remove empty line

npawar

looking good so far! minior comments

npawar · 2022-04-22T00:40:07Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

@@ -492,18 +498,22 @@ public SuccessResponse updateTableConfig(
  @ApiOperation(value = "Validate table config for a table",
      notes = "This API returns the table config that matches the one you get from 'GET /tables/{tableName}'."
          + " This allows us to validate table config before apply.")
-  public String checkTableConfig(
+  public ObjectNode checkTableConfig(


should we take this opportunity to also wrap this into a ConfigValidationResponse object?

I checked this. But it's getting a little tricky to do it in a backward compatible way. Since ConfigSuccessResponse extends SuccesResponse while this function returns a custom object..

npawar · 2022-04-22T00:45:04Z

pinot-spi/src/test/java/org/apache/pinot/spi/utils/TestClass.java

+
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class TestClass {


should this just be inside JsonUtilsTest? It's very non-generic to be by itself

Jackson is not able to work with inner classes. Hence had added it here.
Renamed to something more relevant

npawar · 2022-04-22T00:48:03Z

...-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java

-      ControllerTestUtils
-          .sendPostRequest(StringUtil.join("/", ControllerTestUtils.getControllerBaseApiUrl(), "tables", "validate"),
-              offlineTableConfig.toJsonString());
+      ControllerTestUtils.sendPostRequest(


would be nice to have couple of tests in validate.

validation success, but unparseable props

validation failed
to make sure both these experiences are unaffected by this change
(optional) test case in add and update too

Added unrecognizedProperties tests for add, update and validate table APIs.
Validation failed tests already exist for all APIs.

npawar

lgtm

Jackie-Jiang

LGTM. Only minor comments

Jackie-Jiang · 2022-04-26T23:02:48Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java


  public static <T> T stringToObject(String jsonString, Class<T> valueType)
      throws IOException {
    return DEFAULT_READER.forType(valueType).readValue(jsonString);
  }

+  public static <T> Pair<T, Map<String, Object>> stringToObjectAndUnparseableProps(String jsonString,


Suggest renaming

Suggested change

public static <T> Pair<T, Map<String, Object>> stringToObjectAndUnparseableProps(String jsonString,

public static <T> Pair<T, Map<String, Object>> stringToObjectAndUnrecognizedProperties(String jsonString,

Jackie-Jiang · 2022-04-26T23:03:28Z

pinot-spi/src/test/java/org/apache/pinot/spi/utils/JsonUtilsTest.java

@@ -273,6 +274,20 @@ public void testFlatten()
    }
  }

+  @Test
+  public void testUnparseableJson() throws Exception {


Suggest renaming

Suggested change

public void testUnparseableJson() throws Exception {

public void testUnrecognizedProperties() throws Exception {

Jackie-Jiang · 2022-04-26T23:04:19Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

      String tableConfigStr,
      @ApiParam(value = "comma separated list of validation type(s) to skip. supported types: (ALL|TASK|UPSERT)")
      @QueryParam("validationTypesToSkip") @Nullable String typesToSkip, @Context HttpHeaders httpHeaders,
      @Context Request request) {
    // TODO introduce a table config ctor with json string.
+    Pair<TableConfig, Map<String, Object>> tableConfigAndUnparsedProps;


Suggest renaming

Suggested change

Pair<TableConfig, Map<String, Object>> tableConfigAndUnparsedProps;

Pair<TableConfig, Map<String, Object>> tableConfigAndUnrecognizedProperties;

This PR is a follow up of #8514. It adds the "unrecognizedProperties" fields to /schemas and /tableConfigs APIs

KKcorps requested a review from npawar April 13, 2022 10:35

saurabhd336 changed the title ~~Indicate what fields are getting ignored when adding configs (WIP)~~ Indicate what fields are getting ignored when adding configs Apr 14, 2022

saurabhd336 force-pushed the configJsonIgnoreNotify branch 10 times, most recently from 8980030 to 55f7d2b Compare April 21, 2022 06:44

Jackie-Jiang reviewed Apr 21, 2022

View reviewed changes

npawar reviewed Apr 22, 2022

View reviewed changes

saurabhd336 requested review from npawar and Jackie-Jiang April 22, 2022 05:58

saurabhd336 force-pushed the configJsonIgnoreNotify branch from fd9519d to 0f9cea5 Compare April 24, 2022 05:46

npawar approved these changes Apr 25, 2022

View reviewed changes

Jackie-Jiang approved these changes Apr 26, 2022

View reviewed changes

saurabhd336 force-pushed the configJsonIgnoreNotify branch from 0f9cea5 to 00d6277 Compare April 27, 2022 04:35

Saurabh Dubey and others added 8 commits April 27, 2022 15:43

Add json parsing method

add4dca

Flatten before comparing

f9e01c3

API response chagnes

edbcf0d

Linting

c8360b5

Fix json formatting in respone

74e5b62

Minor fixes

28c3a24

Change API response classes

5154536

Change all table APIs

38feece

Saurabh Dubey added 8 commits April 27, 2022 15:43

Review comments

a3dae9a

Formatting

ce594d0

Fix tests

7df35bb

Formatting

4c827ad

Fix tests

d698df3

Formatting

c084a3e

Review comments

36dcf03

Review comments

6fa3e6a

saurabhd336 force-pushed the configJsonIgnoreNotify branch from 00d6277 to 2759e75 Compare April 27, 2022 10:14

Review comments

d634396

saurabhd336 force-pushed the configJsonIgnoreNotify branch from 2759e75 to d634396 Compare April 27, 2022 11:31

saurabhd336 requested review from npawar and Jackie-Jiang April 27, 2022 16:52

npawar merged commit b38a59b into apache:master Apr 27, 2022

saurabhd336 mentioned this pull request Apr 28, 2022

Add unrecognizedProperties to schema and tableConfigs APIs #8606

Merged

npawar pushed a commit that referenced this pull request May 3, 2022

Add unrecognizedProperties to schema and tableConfigs APIs (#8606)

4e14101

This PR is a follow up of #8514. It adds the "unrecognizedProperties" fields to /schemas and /tableConfigs APIs

saurabhd336 mentioned this pull request May 3, 2022

Indicate what fields are getting ignored when adding configs #8318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indicate what fields are getting ignored when adding configs #8514

Indicate what fields are getting ignored when adding configs #8514

saurabhd336 commented Apr 12, 2022 •

edited

Loading

saurabhd336 commented Apr 12, 2022

codecov-commenter commented Apr 13, 2022 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Apr 21, 2022

saurabhd336 Apr 22, 2022

Jackie-Jiang Apr 21, 2022

Jackie-Jiang Apr 21, 2022

saurabhd336 Apr 22, 2022 •

edited

Loading

Jackie-Jiang Apr 22, 2022

saurabhd336 Apr 24, 2022

Jackie-Jiang Apr 21, 2022

saurabhd336 Apr 22, 2022

Jackie-Jiang Apr 21, 2022

saurabhd336 Apr 22, 2022

npawar left a comment •

edited

Loading

npawar Apr 22, 2022

saurabhd336 Apr 22, 2022

npawar Apr 22, 2022

saurabhd336 Apr 22, 2022

npawar Apr 22, 2022

saurabhd336 Apr 22, 2022

npawar left a comment

Jackie-Jiang left a comment

Jackie-Jiang Apr 26, 2022

saurabhd336 Apr 27, 2022

Jackie-Jiang Apr 26, 2022

saurabhd336 Apr 27, 2022

Jackie-Jiang Apr 26, 2022

saurabhd336 Apr 27, 2022

	private final Map<String, Object> _unparseableProps;
	private final Map<String, Object> _ unrecognizedProperties;

		}

		private static Stream<Map.Entry<String, Object>> flatten(Map.Entry<String, Object> entry) {



		@JsonIgnoreProperties(ignoreUnknown = true)
		public class TestClass {

	public static <T> Pair<T, Map<String, Object>> stringToObjectAndUnparseableProps(String jsonString,
	public static <T> Pair<T, Map<String, Object>> stringToObjectAndUnrecognizedProperties(String jsonString,

	public void testUnparseableJson() throws Exception {
	public void testUnrecognizedProperties() throws Exception {

	Pair<TableConfig, Map<String, Object>> tableConfigAndUnparsedProps;
	Pair<TableConfig, Map<String, Object>> tableConfigAndUnrecognizedProperties;

Indicate what fields are getting ignored when adding configs #8514

Indicate what fields are getting ignored when adding configs #8514

Conversation

saurabhd336 commented Apr 12, 2022 • edited Loading

saurabhd336 commented Apr 12, 2022

codecov-commenter commented Apr 13, 2022 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhd336 Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npawar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npawar left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhd336 commented Apr 12, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

saurabhd336 Apr 22, 2022 •

edited

Loading

npawar left a comment •

edited

Loading