You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/08/07 14:56:01 UTC
[GitHub] [helix] rabashizade opened a new pull request #1229: Add JarLoader interface and LocalJarLoader class
rabashizade opened a new pull request #1229:
URL: https://github.com/apache/helix/pull/1229
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1228
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
This PR adds JarLoader interface and LocalJarLoader class which implements this interface to load JAR files that contain task classes which we want to dynamically load in the Participant.
### Tests
- [ ] The following tests are written for this issue:
No tests were written, as this PR only adds an interface and a class to the project.
- [x] The following is the result of the "mvn test" command on the appropriate module:
Since the PR doesn't modify any existing code, it's not going to cause any test failures. The following is the build result:
```
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Helix 1.0.2-SNAPSHOT:
[INFO]
[INFO] Apache Helix ....................................... SUCCESS [ 2.936 s]
[INFO] Apache Helix :: Metrics Common ..................... SUCCESS [ 3.749 s]
[INFO] Apache Helix :: Metadata Store Directory Common .... SUCCESS [ 2.363 s]
[INFO] Apache Helix :: ZooKeeper API ...................... SUCCESS [ 5.089 s]
[INFO] Apache Helix :: Helix Common ....................... SUCCESS [ 3.137 s]
[INFO] Apache Helix :: Core ............................... SUCCESS [ 36.918 s]
[INFO] Apache Helix :: Admin Webapp ....................... SUCCESS [ 6.789 s]
[INFO] Apache Helix :: Restful Interface .................. SUCCESS [ 11.578 s]
[INFO] Apache Helix :: Distributed Lock ................... SUCCESS [ 1.888 s]
[INFO] Apache Helix :: HelixAgent ......................... SUCCESS [ 2.996 s]
[INFO] Apache Helix :: Recipes ............................ SUCCESS [ 0.038 s]
[INFO] Apache Helix :: Recipes :: Rabbitmq Consumer Group . SUCCESS [ 1.241 s]
[INFO] Apache Helix :: Recipes :: Rsync Replicated File Store SUCCESS [ 1.386 s]
[INFO] Apache Helix :: Recipes :: distributed lock manager SUCCESS [ 1.019 s]
[INFO] Apache Helix :: Recipes :: distributed task execution SUCCESS [ 1.131 s]
[INFO] Apache Helix :: Recipes :: service discovery ....... SUCCESS [ 1.058 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:24 min
[INFO] Finished at: 2020-08-07T07:52:04-07:00
[INFO] ------------------------------------------------------------------------
```
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467162922
##########
File path: helix-core/src/main/java/org/apache/helix/task/DynamicTaskConfig.java
##########
@@ -0,0 +1,115 @@
+package org.apache.helix.task;
+
+/*
+ * 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.
+ */
+
+import java.util.List;
+
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+
+/**
+ * A wrapper class for ZNRecord, used to store configs for tasks that are to be dynamically loaded
+ */
+public class DynamicTaskConfig {
Review comment:
Why is this part of this PR? I thought you had created a PR for this already?
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskConstants.java
##########
@@ -49,4 +49,24 @@
public static final String PREV_RA_NODE = "PreviousResourceAssignment";
public static final boolean DEFAULT_TASK_ENABLE_COMPRESSION = false;
+ /**
+ * Name of the JAR file for the task dynamically loaded in {@link TaskStateModel}
+ */
+ public static final String TASK_JAR_FILE_KEY = "JAR_FILE";
Review comment:
Why is this change part of this PR?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467166467
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
Review comment:
Do you mean to add the example URL to the code? An example is: `file:/home/rabashiz/apache-helix/helix/helix-core/src/test/resources/Reindex.jar`
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #1229:
URL: https://github.com/apache/helix/pull/1229
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467163024
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskConstants.java
##########
@@ -49,4 +49,24 @@
public static final String PREV_RA_NODE = "PreviousResourceAssignment";
public static final boolean DEFAULT_TASK_ENABLE_COMPRESSION = false;
+ /**
+ * Name of the JAR file for the task dynamically loaded in {@link TaskStateModel}
+ */
+ public static final String TASK_JAR_FILE_KEY = "JAR_FILE";
Review comment:
Why is this change part of this PR? Did you rebase from `https://github.com/apache/helix/tree/dynamically-loaded-task`?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r469296011
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,53 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ // If taskJarFile doesn't exist or it's a directory, throw exception
+ File taskJarFile = new File(jarPath);
+ if (!taskJarFile.exists() || taskJarFile.isDirectory()) {
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
Review comment:
That's a good point, but I think later checks, when we actually try to read from this JAR, will throw appropriate exceptions if there are issues with permissions.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467164328
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
Review comment:
Question: What would the returned URL look like for a local JAR?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467227495
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
Review comment:
Thank you. That makes sense.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r468164516
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,53 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ // If taskJarFile doesn't exist or it's a directory, throw exception
+ File taskJarFile = new File(jarPath);
+ if (!taskJarFile.exists() || taskJarFile.isDirectory()) {
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
Review comment:
Wouldn't IllegalArgument be more appropriate?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] manick02 commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
manick02 commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r468308234
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,53 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ // If taskJarFile doesn't exist or it's a directory, throw exception
+ File taskJarFile = new File(jarPath);
+ if (!taskJarFile.exists() || taskJarFile.isDirectory()) {
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
Review comment:
Does it make sense to check for any other permission like whether jar file is readable etc here to make sure jar is readable for JVM to execute?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467165124
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ File taskJarFile;
+ try {
+ taskJarFile = new File(jarPath);
+
+ // If taskJarFile exists and it's not a directory, return its URL
+ if (taskJarFile.exists() && !taskJarFile.isDirectory()) {
+ return taskJarFile.toURI().toURL();
+ }
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
Review comment:
Let's refactor this. The way it's written is slightly awkward:
Let's use the following format:
```
if (error condition is met) {
throw Exception;
}
return result;
```
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467165835
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ File taskJarFile;
+ try {
+ taskJarFile = new File(jarPath);
+
+ // If taskJarFile exists and it's not a directory, return its URL
+ if (taskJarFile.exists() && !taskJarFile.isDirectory()) {
+ return taskJarFile.toURI().toURL();
+ }
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
+ } catch (MalformedURLException e) {
Review comment:
It seems that `new File()` only throws
NullPointerException - If uri is null
IllegalArgumentException - If the preconditions on the parameter do not hold
per https://docs.oracle.com/javase/7/docs/api/java/io/File.html.
Why are we catching `MalformedURLException` that's not thrown by anyone?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on pull request #1229:
URL: https://github.com/apache/helix/pull/1229#issuecomment-672905152
This PR is ready to be merged.
Final commit message:
**Add JarLoader interface and LocalJarLoader implementation**
Adds JarLoader interface to load JAR files that contain task classes
which we want to dynamically load.
Also adds LocalJarLoader implementation of JarLoader interface which
loads a JAR file from a local directory.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467164328
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
Review comment:
Question: What would the returned URL look like for a local JAR? Could you give some examples here?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467164257
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskConstants.java
##########
@@ -49,4 +49,24 @@
public static final String PREV_RA_NODE = "PreviousResourceAssignment";
public static final boolean DEFAULT_TASK_ENABLE_COMPRESSION = false;
+ /**
+ * Name of the JAR file for the task dynamically loaded in {@link TaskStateModel}
+ */
+ public static final String TASK_JAR_FILE_KEY = "JAR_FILE";
Review comment:
Sorry I created the local JarLoader branch from my local DynamicTaskConfig branch. I thought the two (rebasing and creating from latest local branch) would be the same but apparently they're not. Let me rebase and push again.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r468164765
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,53 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ // If taskJarFile doesn't exist or it's a directory, throw exception
+ File taskJarFile = new File(jarPath);
+ if (!taskJarFile.exists() || taskJarFile.isDirectory()) {
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
+ }
+
+ try {
+ return taskJarFile.toURI().toURL();
+ } catch (MalformedURLException e) {
+ LOG.error("Failed to open JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("Malformed JAR URL for task");
Review comment:
IllegalArgument since the input is malformed?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467164257
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskConstants.java
##########
@@ -49,4 +49,24 @@
public static final String PREV_RA_NODE = "PreviousResourceAssignment";
public static final boolean DEFAULT_TASK_ENABLE_COMPRESSION = false;
+ /**
+ * Name of the JAR file for the task dynamically loaded in {@link TaskStateModel}
+ */
+ public static final String TASK_JAR_FILE_KEY = "JAR_FILE";
Review comment:
Sorry I created the local JarLoader branch from my local DynamicTaskConfig branch. I thought the two would be the same but apparently they're not. Let me rebase and push again.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467166467
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
Review comment:
Do you mean to add the example URL to the comment in code? An example is: `file:/home/rabashiz/apache-helix/helix/helix-core/src/test/resources/Reindex.jar`
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1229: Add JarLoader interface and LocalJarLoader class
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1229:
URL: https://github.com/apache/helix/pull/1229#discussion_r467167354
##########
File path: helix-core/src/main/java/org/apache/helix/task/api/LocalJarLoader.java
##########
@@ -0,0 +1,54 @@
+package org.apache.helix.task.api;
+
+/*
+ * 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.
+ */
+
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LocalJarLoader implements JarLoader {
+ private static final Logger LOG = LoggerFactory.getLogger(LocalJarLoader.class);
+
+ /**
+ * Loads a local JAR file and returns its URL.
+ * @param jarPath path of the JAR file
+ * @return URL of the JAR file at path jarPath
+ */
+ @Override
+ public URL loadJar(String jarPath) {
+ File taskJarFile;
+ try {
+ taskJarFile = new File(jarPath);
+
+ // If taskJarFile exists and it's not a directory, return its URL
+ if (taskJarFile.exists() && !taskJarFile.isDirectory()) {
+ return taskJarFile.toURI().toURL();
+ }
+ LOG.error("Failed to find JAR " + jarPath + " for new task.");
+ throw new IllegalStateException("No JAR for task");
+ } catch (MalformedURLException e) {
Review comment:
`taskJarFile.toURI().toURL()` throws it. I'll re-organize the code to make that clearer.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org