Jackie-Jiang commented on a change in pull request #6424:
URL: https://github.com/apache/incubator-pinot/pull/6424#discussion_r648746685
##########
File path:
pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/ExternalViewReaderTest.java
##########
@@ -75,7 +75,7 @@ public void testGetLiveBrokersExceptionState() throws
IOException {
// Setup
final List<String> expectedResult = Arrays.asList();
when(mockZkClient.readData(Mockito.anyString(),
Mockito.anyBoolean())).thenThrow(
- IOException.class);
+ RuntimeException.class);
Review comment:
Is this related?
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java
##########
@@ -36,18 +35,16 @@
public PluginClassLoader(URL[] urls, ClassLoader parent) {
super(urls, parent);
classLoader = PluginClassLoader.class.getClassLoader();
- Method method = null;
- try {
- method = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);
-
- } catch (NoSuchMethodException e) {
- //this should never happen
- ExceptionUtils.rethrow(e);
- }
- method.setAccessible(true);
for (URL url : urls) {
try {
- method.invoke(classLoader, url);
+ /**
+ * ClassLoader in java9+ does not extend URLClassLoader.
Review comment:
Don't quite understand the comment here. Do you mean `URLClassLoader`
does not extend `ClassLoader` in java 9? But it should still have the method
`addURL()` right?
##########
File path:
pinot-common/src/test/java/org/apache/pinot/common/utils/PinotDataTypeTest.java
##########
@@ -105,6 +105,7 @@ public void testBytes() {
@Test
public void testTimestamp() {
Timestamp timestamp = Timestamp.valueOf(LocalDateTime.now());
+ timestamp.setNanos(0);
Review comment:
Is this related?
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -317,7 +317,7 @@ minion:
readinessEnabled: false
dataDir: /var/pinot/minion/data
- jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200
-XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime
-XX:+PrintGCApplicationConcurrentTime -Xloggc:/opt/pinot/gc-pinot-minion.log"
+ jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC
-Xlog:gc*,safepoint:gc.log:time,uptime -Xlog:gc:/opt/pinot/gc-pinot-minion.log"
Review comment:
Same here
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java
##########
@@ -36,18 +35,16 @@
public PluginClassLoader(URL[] urls, ClassLoader parent) {
super(urls, parent);
classLoader = PluginClassLoader.class.getClassLoader();
- Method method = null;
- try {
- method = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);
-
- } catch (NoSuchMethodException e) {
- //this should never happen
- ExceptionUtils.rethrow(e);
- }
- method.setAccessible(true);
for (URL url : urls) {
try {
- method.invoke(classLoader, url);
+ /**
+ * ClassLoader in java9+ does not extend URLClassLoader.
+ * If the class is not found in the parent classloader,
+ * it will be found in this classloader via findClass().
+ *
+ * @see
https://community.oracle.com/tech/developers/discussion/4011800/base-classloader-no-longer-from-urlclassloader
+ */
+ super.addURL(url);
Review comment:
The try-catch can be removed
```suggestion
addURL(url);
```
##########
File path: kubernetes/helm/pinot/values.yaml
##########
@@ -152,7 +152,7 @@ broker:
# fsGroup: 2000
securityContext: {}
- jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200
-XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime
-XX:+PrintGCApplicationConcurrentTime -Xloggc:/opt/pinot/gc-pinot-broker.log"
+ jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC
-Xlog:gc*,safepoint:gc.log:time,uptime -Xlog:gc:/opt/pinot/gc-pinot-broker.log"
Review comment:
Do we want to remove the `-XX:MaxGCPauseMillis=200`?
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
