Ian Maxon has posted comments on this change. Change subject: ASTERIXDB-1714: Eliminate dependency on org.json ......................................................................
Patch Set 13: (7 comments) I just left it changed because it had equivalent content, but I can change it back if it's an issue. I'll get to replacing put and aligning the versions in the meantime. https://asterix-gerrit.ics.uci.edu/#/c/1392/13/asterixdb/asterix-app/pom.xml File asterixdb/asterix-app/pom.xml: Line 333: </dependency> > This seems to be a duplicate. Sure, I can make it managed. https://asterix-gerrit.ics.uci.edu/#/c/1392/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ClusterAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ClusterAPIServlet.java: Line 134: ObjectMapper om = new ObjectMapper(); > Could we create this directly before we use it? You mean just like, new ObjectMapper().createObjectNode? Some of the oddness here is also related to the way I went about this (search/replace) https://asterix-gerrit.ics.uci.edu/#/c/1392/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/QueryServiceServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/QueryServiceServlet.java: Line 314: SessionConfig.ResultDecorator handlePrefix = new SessionConfig.ResultDecorator() { > Why did we need this change? The handle was being handled inappropriately ( :D ) Line 402: addStack); > Could be put this back? Or does it cause problems? I could, but it was never executed, so I thought I would remove it https://asterix-gerrit.ics.uci.edu/#/c/1392/13/asterixdb/asterix-app/src/test/resources/runtimets/results/api/cluster_state_1/cluster_state_1.1.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/api/cluster_state_1/cluster_state_1.1.adm: Line 9: "cluster.partitions" : { > The content looks different here. Was that a conscious decision? Only in so far as I saw the difference, and it looked functionally equivalent to me in terms of just reading it. I can make it the same if there's some tool that it using it at the moment. https://asterix-gerrit.ics.uci.edu/#/c/1392/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/JSONUtil.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/JSONUtil.java: Line 45: public static String convertNode(final JsonNode node) throws JsonProcessingException { > This looks pretty involved. Could you add a short comment what's special he Sure, it's just to sort a node so it always gets outputted the same way for test purposes. https://asterix-gerrit.ics.uci.edu/#/c/1392/13/asterixdb/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java File asterixdb/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java: Line 549: e.printStackTrace(); > I think that we should wrap and throw the exception here. Generally with servlets though you don't want to throw exceptions to the top right? -- To view, visit https://asterix-gerrit.ics.uci.edu/1392 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie9c5400fd134ae75d43385255af7794e968b1c7e Gerrit-PatchSet: 13 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
