You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by "rzo1 (via GitHub)" <gi...@apache.org> on 2023/08/28 07:42:03 UTC
[PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
rzo1 opened a new pull request, #3572:
URL: https://github.com/apache/storm/pull/3572
## What is the purpose of the change
- Removes `com.googlecode.json-simple:json-simple` from Storm core / runtime modules (legacy library 12 years old) and replace it with already included json minidev library.
- This replacement results in some code changes across the project
- Minidev json is now included in the client shaded deps.
- Note: `storm-core` clojure integration test relies on `json-simple` classes to create a JSON structure and is highly library dependent. As I am not speaking clojure good enough, it might be a separate issue to replace the library in the test setup, too.
- Removes `org.jooq:jool` from webui as it was included for the purpose of a few methods which can be replaced with Java 11 features.
- Pins some versions on parent pom to avoid inclusion of the same library in the distribution
- commons-lang3 -> 3.13.0
- log4j -> 2.20.0
- slf4j -> 1.7.36
- Jersey -> 2.40
- Dropwizard -> 1.3.29
- j2html -> 1.6.0
- minidev json -> 2.5.0
- JUnit 5.10.0
- Excludes some garbage from autocreds as jdbc/mssql driver is not needed for runtime operations
If this reviewed, I will create Jira issues for every version upgrade included in this PR in order to have them in the next release logs.
## How was the change tested
- GH actions
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on code in PR #3572:
URL: https://github.com/apache/storm/pull/3572#discussion_r1309795216
##########
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java:
##########
@@ -333,11 +330,16 @@ public void testNextByteOffsetsAreCorrectForEachMatch() throws Exception {
dataAndExpected.add(new Tuple3<>(12, 12, null));
dataAndExpected.add(new Tuple3<>(13, 12, null));
- dataAndExpected.forEach(Unchecked.consumer(data -> {
- Map<String, Object> result = handler.substringSearch(file.toPath(), pattern, data.v1());
- assertEquals(data.v3(), result.get("nextByteOffset"));
- assertEquals(data.v2().intValue(), ((List) result.get("matches")).size());
- }));
+ dataAndExpected.forEach(data -> {
+ Map<String, Object> result;
+ try {
+ result = handler.substringSearch(file.toPath(), pattern, data.value1);
+ assertEquals(data.value3, result.get("nextByteOffset"));
+ assertEquals(data.value2.intValue(), ((List) result.get("matches")).size());
+ } catch (InvalidRequestException e) {
+ throw new RuntimeException(e);
Review Comment:
This is actually a unit test, so no regression here for older code. I think you are referring to the JOOQ removal from `Unchecked.function(...)` to `try-catch`.
`Unchecked.function(...)` is implemented as:
```bash
try {
return function.apply(t);
}
catch (Throwable e) {
handler.accept(e);
throw new IllegalStateException("Exception handler must throw a RuntimeException", e);
}
```
so I think we are good here. The exception handler is `THROWABLE_TO_RUNTIME_EXCEPTION` :-)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "bipinprasad (via GitHub)" <gi...@apache.org>.
bipinprasad merged PR #3572:
URL: https://github.com/apache/storm/pull/3572
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "bipinprasad (via GitHub)" <gi...@apache.org>.
bipinprasad commented on code in PR #3572:
URL: https://github.com/apache/storm/pull/3572#discussion_r1309769830
##########
storm-server/src/main/java/org/apache/storm/scheduler/resource/normalization/NormalizedResourceRequest.java:
##########
@@ -20,15 +20,15 @@
import java.util.HashMap;
import java.util.Map;
+import net.minidev.json.JSONObject;
+import net.minidev.json.parser.JSONParser;
Review Comment:
Thanks for this pull request. I believe org.json.simple.parser.JSONParser was not always thread safe.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #3572:
URL: https://github.com/apache/storm/pull/3572#issuecomment-1704371786
Created the related dependency upgrade jiras + jira for the json simple replacement, so we have it in the changelog for 2.6.0
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "bipinprasad (via GitHub)" <gi...@apache.org>.
bipinprasad commented on code in PR #3572:
URL: https://github.com/apache/storm/pull/3572#discussion_r1309762319
##########
pom.xml:
##########
@@ -854,6 +855,11 @@
<artifactId>javax.servlet-api</artifactId>
<version>${servlet.version}</version>
</dependency>
+ <dependency>
Review Comment:
Do we still need glassfish?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on code in PR #3572:
URL: https://github.com/apache/storm/pull/3572#discussion_r1309795216
##########
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java:
##########
@@ -333,11 +330,16 @@ public void testNextByteOffsetsAreCorrectForEachMatch() throws Exception {
dataAndExpected.add(new Tuple3<>(12, 12, null));
dataAndExpected.add(new Tuple3<>(13, 12, null));
- dataAndExpected.forEach(Unchecked.consumer(data -> {
- Map<String, Object> result = handler.substringSearch(file.toPath(), pattern, data.v1());
- assertEquals(data.v3(), result.get("nextByteOffset"));
- assertEquals(data.v2().intValue(), ((List) result.get("matches")).size());
- }));
+ dataAndExpected.forEach(data -> {
+ Map<String, Object> result;
+ try {
+ result = handler.substringSearch(file.toPath(), pattern, data.value1);
+ assertEquals(data.value3, result.get("nextByteOffset"));
+ assertEquals(data.value2.intValue(), ((List) result.get("matches")).size());
+ } catch (InvalidRequestException e) {
+ throw new RuntimeException(e);
Review Comment:
This is actually a unit test, so no regression here for older code. I think you are referring to the JOOQ removal from `Unchecked.function(...)` to `try-catch`.
`Unchecked.function(...)` is implemented as:
```bash
try {
return function.apply(t);
}
catch (Throwable e) {
handler.accept(e);
throw new IllegalStateException("Exception handler must throw a RuntimeException", e);
}
```
so I think we are good here.
##########
pom.xml:
##########
@@ -854,6 +855,11 @@
<artifactId>javax.servlet-api</artifactId>
<version>${servlet.version}</version>
</dependency>
+ <dependency>
Review Comment:
I pulled it up to avoid having multiple `el` versions in the classpath and override the two versions we previously had. Actually, we should get rid of the `javax.*` namespace in the long run and migrate to `jakarta.*` anyway. In this process, we can switch to the Eclipse foundation spec / api jars which are good to use in an ASF project.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] STORM-3961 - Modernize Storm-UI Dependencies (storm)
Posted by "bipinprasad (via GitHub)" <gi...@apache.org>.
bipinprasad commented on code in PR #3572:
URL: https://github.com/apache/storm/pull/3572#discussion_r1309775684
##########
storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogSearchHandlerTest.java:
##########
@@ -333,11 +330,16 @@ public void testNextByteOffsetsAreCorrectForEachMatch() throws Exception {
dataAndExpected.add(new Tuple3<>(12, 12, null));
dataAndExpected.add(new Tuple3<>(13, 12, null));
- dataAndExpected.forEach(Unchecked.consumer(data -> {
- Map<String, Object> result = handler.substringSearch(file.toPath(), pattern, data.v1());
- assertEquals(data.v3(), result.get("nextByteOffset"));
- assertEquals(data.v2().intValue(), ((List) result.get("matches")).size());
- }));
+ dataAndExpected.forEach(data -> {
+ Map<String, Object> result;
+ try {
+ result = handler.substringSearch(file.toPath(), pattern, data.value1);
+ assertEquals(data.value3, result.get("nextByteOffset"));
+ assertEquals(data.value2.intValue(), ((List) result.get("matches")).size());
+ } catch (InvalidRequestException e) {
+ throw new RuntimeException(e);
Review Comment:
Will the behaviour with RuntimeException be similar to old code?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org