You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by chandnisingh <gi...@git.apache.org> on 2015/11/17 10:18:08 UTC
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
GitHub user chandnisingh opened a pull request:
https://github.com/apache/incubator-apex-core/pull/171
APEX-268 #resolve
@davidyan74 please merge. Please note the change is attributed to jenkins
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chandnisingh/incubator-apex-core APEX-268
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-apex-core/pull/171.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #171
----
commit 3e0c4cb10e7e87da0b6d8fc3e517ed8ebcfaeac9
Author: MalharJenkins <je...@datatorrent.com>
Date: 2015-11-16T21:43:31Z
APEX-268 #resolve
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45075876
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -70,15 +77,13 @@ public void onMessage(String message)
PubSubMessage<Object> pubSubMessage;
try {
pubSubMessage = codec.parseMessage(message);
- PubSubWebSocketClient.this.onMessage(pubSubMessage.getType().getIdentifier(), pubSubMessage.getTopic(), pubSubMessage.getData());
- }
- catch (JsonParseException jpe) {
+ PubSubWebSocketClient.this
+ .onMessage(pubSubMessage.getType().getIdentifier(), pubSubMessage.getTopic(), pubSubMessage.getData());
--- End diff --
Consider introducing local final variables.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45100607
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -376,7 +385,8 @@ public void unsubscribe(String topic) throws IOException
* @return subscribe num subscriber message.
* @throws IOException
*/
- public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws IOException
+ public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws
+ IOException
--- End diff --
The line break here should probably be before the throw keyword
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45072891
--- Diff: common/src/main/java/com/datatorrent/common/util/SerializableObject.java ---
@@ -58,37 +59,23 @@ public Object readResolve() throws ObjectStreamException
Constructor<? extends SerializableObject> constructor = this.getClass().getConstructor(this.getClass());
try {
constructor.setAccessible(true);
- }
- catch (SecurityException ex) {
+ } catch (SecurityException ex) {
logger.warn("Accessing copy constructor {} failed.", constructor, ex);
}
try {
return constructor.newInstance(this);
- }
- catch (InstantiationException ex) {
- throw new RuntimeException("Instantiation using copy constructor failed!", ex);
- }
- catch (IllegalAccessException ex) {
- throw new RuntimeException("Instantiation using copy constructor failed!", ex);
- }
- catch (IllegalArgumentException ex) {
- throw new RuntimeException("Instantiation using copy constructor failed!", ex);
- }
- catch (InvocationTargetException ex) {
+ } catch (InstantiationException | IllegalAccessException | IllegalArgumentException |
+ InvocationTargetException ex) {
throw new RuntimeException("Instantiation using copy constructor failed!", ex);
}
- }
- catch (NoSuchMethodException snme) {
- logger.debug("No copy constructor detected for class {}, trying default constructor.", this.getClass().getSimpleName());
+ } catch (NoSuchMethodException snme) {
+ logger.debug("No copy constructor detected for class {}, trying default constructor.", this.getClass()
+ .getSimpleName());
--- End diff --
Can it be wrapped on space and not on dot?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45102033
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -376,7 +385,8 @@ public void unsubscribe(String topic) throws IOException
* @return subscribe num subscriber message.
* @throws IOException
*/
- public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws IOException
+ public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws
+ IOException
--- End diff --
Why?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45105984
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -376,7 +385,8 @@ public void unsubscribe(String topic) throws IOException
* @return subscribe num subscriber message.
* @throws IOException
*/
- public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws IOException
+ public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws
+ IOException
--- End diff --
No it is very hard. You can take up the changes in engine module and try it out .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45103271
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -376,7 +385,8 @@ public void unsubscribe(String topic) throws IOException
* @return subscribe num subscriber message.
* @throws IOException
*/
- public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws IOException
+ public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws
+ IOException
--- End diff --
Consistency and readability. You have other places that break before the throws, and it is odd to break the line after the throws keyword. Imagine something like:
{code}
public class SomeLongClassName extends
SomeOtherLongClassName implements
SomeLongInterfaceNameA
{code}
versus
{code}
public class SomeLongClassName
extends SomeOtherLongClassName
implements SomeLongInterfaceNameA
{code}
In the first case, SomeOtherLongClassName has nothing to do with the implements keyword, and yet they are on the same line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45102044
--- Diff: common/src/main/java/com/datatorrent/common/util/ScheduledThreadPoolExecutor.java ---
@@ -22,13 +22,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
/**
* <p>ScheduledThreadPoolExecutor class.</p>
*
* @since 0.3.2
*/
-public class ScheduledThreadPoolExecutor extends java.util.concurrent.ScheduledThreadPoolExecutor implements ScheduledExecutorService
+public class ScheduledThreadPoolExecutor extends java.util.concurrent.ScheduledThreadPoolExecutor implements
+ ScheduledExecutorService
--- End diff --
Why?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/171#issuecomment-157465217
@vrozov @davidyan74 I have addressed the review comments. However I don't think introducing local variables should be part of fixing checkstyle violations.
If this is more about fixing read-ability of code (not just style violations) then I don't think that I can take up this task.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh closed the pull request at:
https://github.com/apache/incubator-apex-core/pull/171
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45074939
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -201,40 +208,43 @@ public void openConnectionAsync() throws IOException
try {
json.put("userName", userName);
json.put("password", password);
- }
- catch (JSONException ex) {
+ } catch (JSONException ex) {
throw new RuntimeException(ex);
}
- client.preparePost(loginUrl).setHeader("Content-Type", "application/json").setBody(json.toString()).execute(new AsyncCompletionHandler<Response>()
- {
-
- @Override
- public Response onCompleted(Response response) throws Exception
- {
- List<Cookie> cookies = response.getCookies();
- BoundRequestBuilder brb = client.prepareGet(uri.toString());
- if (cookies != null) {
- for (Cookie cookie : cookies) {
- brb.addCookie(cookie);
+ client.preparePost(loginUrl).setHeader("Content-Type", "application/json").setBody(json.toString())
--- End diff --
Consider introducing local variable
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45070227
--- Diff: common/src/main/java/com/datatorrent/common/experimental/AppData.java ---
@@ -60,16 +62,18 @@
}
/**
- * This interface represents a query operator which can be embedded into an AppData data source. This operator could also
- * be used as a standalone operator. The distinction between being used in a standalone or embedded context is made by
- * the {@link EmbeddableQueryInfoProvider#enableEmbeddedMode} method. If this method is called at least once then the {@link EmbeddableQueryInfoProvider}
- * will operate as if it were embedded in an {@link AppData.Store} operator. If this method is never called then the operator will behave as if
+ * This interface represents a query operator which can be embedded into an AppData data source. This operator
+ * could also be used as a standalone operator. The distinction between being used in a standalone or embedded
+ * context is made by the {@link EmbeddableQueryInfoProvider#enableEmbeddedMode} method.
+ * If this method is called at least once then the {@link EmbeddableQueryInfoProvider} will operate as if it were
+ * embedded in an {@link AppData.Store} operator. If this method is never called then the operator will behave as if
* it were a standalone operator.<br/><br/>
- * <b>Note:</b> When an {@link EmbeddableQueryInfoProvider} is set on an {@link AppData.Store} then it's {@link EmbeddableQueryInfoProvider#enableEmbeddedMode}
- * method is called before {@link Operator#setup}.
+ * <b>Note:</b> When an {@link EmbeddableQueryInfoProvider} is set on an {@link AppData.Store} then it's
+ * {@link EmbeddableQueryInfoProvider#enableEmbeddedMode} method is called before {@link Operator#setup}.
* @param <QUERY_TYPE> The type of the query emitted by the operator.
*/
- interface EmbeddableQueryInfoProvider<QUERY_TYPE> extends Operator, ConnectionInfoProvider, Operator.ActivationListener<OperatorContext>
+ interface EmbeddableQueryInfoProvider<QUERY_TYPE> extends Operator, ConnectionInfoProvider, Operator
+ .ActivationListener<OperatorContext>
--- End diff --
It is more readable to wrap on space, not on dot
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45105484
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -376,7 +385,8 @@ public void unsubscribe(String topic) throws IOException
* @return subscribe num subscriber message.
* @throws IOException
*/
- public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws IOException
+ public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws
+ IOException
--- End diff --
That's fine. But I think as you make the style changes, it's not hard to place the line breaks in where they should be, since you're making the changes anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45074311
--- Diff: common/src/main/java/com/datatorrent/common/util/SerializableObject.java ---
@@ -58,37 +59,23 @@ public Object readResolve() throws ObjectStreamException
Constructor<? extends SerializableObject> constructor = this.getClass().getConstructor(this.getClass());
try {
constructor.setAccessible(true);
- }
- catch (SecurityException ex) {
+ } catch (SecurityException ex) {
logger.warn("Accessing copy constructor {} failed.", constructor, ex);
}
try {
return constructor.newInstance(this);
- }
- catch (InstantiationException ex) {
- throw new RuntimeException("Instantiation using copy constructor failed!", ex);
- }
- catch (IllegalAccessException ex) {
- throw new RuntimeException("Instantiation using copy constructor failed!", ex);
- }
- catch (IllegalArgumentException ex) {
- throw new RuntimeException("Instantiation using copy constructor failed!", ex);
- }
- catch (InvocationTargetException ex) {
+ } catch (InstantiationException | IllegalAccessException | IllegalArgumentException |
--- End diff --
java.lang.ReflectiveOperationException?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45076300
--- Diff: common/src/main/java/com/datatorrent/common/util/BasicContainerOptConfigurator.java ---
@@ -61,13 +64,13 @@ public String getJVMOptions(List<DAG.OperatorMeta> operatorMetaList)
long xss = 0;
List<Map<String, Object>> jvmOptsList = Lists.newArrayList();
for (DAG.OperatorMeta operatorMeta : operatorMetaList) {
- Map<String, Object> operatorMap = parseJvmOpts(operatorMeta.getValue(Context.OperatorContext.JVM_OPTIONS), operatorMeta.getValue(Context.OperatorContext.MEMORY_MB));
+ Map<String, Object> operatorMap = parseJvmOpts(operatorMeta
--- End diff --
introduce local final variables?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45100534
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -289,11 +298,11 @@ public void assertUsable() throws IOException
Throwable t = throwable.get();
if (t instanceof IOException) {
throw (IOException)t;
- }
- else {
+ } else {
DTThrowable.rethrow(t);
--- End diff --
Not sure if we should use this opportunity to remove all DTThrowable.rethrow() calls.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45076085
--- Diff: common/src/main/java/com/datatorrent/common/util/FSStorageAgent.java ---
@@ -139,7 +144,8 @@ public Object load(int operatorId, long windowId) throws IOException
@Override
public void delete(int operatorId, long windowId) throws IOException
{
- Path lPath = new Path(path + Path.SEPARATOR + String.valueOf(operatorId) + Path.SEPARATOR + Long.toHexString(windowId));
+ Path lPath = new Path(path + Path.SEPARATOR + String.valueOf(operatorId) + Path.SEPARATOR + Long
+ .toHexString(windowId));
--- End diff --
Wrap on space?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45104998
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -376,7 +385,8 @@ public void unsubscribe(String topic) throws IOException
* @return subscribe num subscriber message.
* @throws IOException
*/
- public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws IOException
+ public static <T> String constructSubscribeNumSubscribersMessage(String topic, PubSubMessageCodec<T> codec) throws
+ IOException
--- End diff --
David I made those changes. However I have some comments here:
About your consistency point: places where throws may have a break before because the method name and arguments maybe long enough.
And as I mentioned before if this task it to ensure readability of classes that I am not the author of, then I shouldn't be making these changes. Individual authors/owners should take them up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45101994
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -289,11 +298,11 @@ public void assertUsable() throws IOException
Throwable t = throwable.get();
if (t instanceof IOException) {
throw (IOException)t;
- }
- else {
+ } else {
DTThrowable.rethrow(t);
--- End diff --
Not part of fixing checkstyle violations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45072581
--- Diff: common/src/test/java/com/datatorrent/common/partitioner/StatelessPartitionerTest.java ---
@@ -94,10 +98,11 @@ public void partition1Test()
DefaultPartition<DummyOperator> defaultPartition = new DefaultPartition<DummyOperator>(dummyOperator);
partitions.add(defaultPartition);
- Collection<Partition<DummyOperator>> newPartitions = statelessPartitioner.definePartitions(partitions, new PartitioningContextImpl(null, 0));
+ Collection<Partition<DummyOperator>> newPartitions = statelessPartitioner
+ .definePartitions(partitions, new PartitioningContextImpl(null, 0));
--- End diff --
Can it be wrapped on space and not on dot?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45075022
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -175,11 +181,11 @@ public void openConnection(long timeoutMillis) throws IOException, ExecutionExce
try {
json.put("userName", userName);
json.put("password", password);
- }
- catch (JSONException ex) {
+ } catch (JSONException ex) {
throw new RuntimeException(ex);
}
- Response response = client.preparePost(loginUrl).setHeader("Content-Type", "application/json").setBody(json.toString()).execute().get();
+ Response response = client.preparePost(loginUrl).setHeader("Content-Type", "application/json")
+ .setBody(json.toString()).execute().get();
--- End diff --
Consider introducing local variable
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45100660
--- Diff: common/src/main/java/com/datatorrent/common/util/ScheduledThreadPoolExecutor.java ---
@@ -22,13 +22,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
/**
* <p>ScheduledThreadPoolExecutor class.</p>
*
* @since 0.3.2
*/
-public class ScheduledThreadPoolExecutor extends java.util.concurrent.ScheduledThreadPoolExecutor implements ScheduledExecutorService
+public class ScheduledThreadPoolExecutor extends java.util.concurrent.ScheduledThreadPoolExecutor implements
+ ScheduledExecutorService
--- End diff --
line break should be before the implements keyword
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45072598
--- Diff: common/src/test/java/com/datatorrent/common/partitioner/StatelessPartitionerTest.java ---
@@ -112,10 +117,11 @@ public void partition5Test()
DefaultPartition<DummyOperator> defaultPartition = new DefaultPartition<DummyOperator>(dummyOperator);
partitions.add(defaultPartition);
- Collection<Partition<DummyOperator>> newPartitions = statelessPartitioner.definePartitions(partitions, new PartitioningContextImpl(null, 0));
+ Collection<Partition<DummyOperator>> newPartitions = statelessPartitioner
+ .definePartitions(partitions, new PartitioningContextImpl(null, 0));
--- End diff --
Can it be wrapped on space and not on dot?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-268 #resolve
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/171#discussion_r45103674
--- Diff: common/src/main/java/com/datatorrent/common/util/PubSubWebSocketClient.java ---
@@ -289,11 +298,11 @@ public void assertUsable() throws IOException
Throwable t = throwable.get();
if (t instanceof IOException) {
throw (IOException)t;
- }
- else {
+ } else {
DTThrowable.rethrow(t);
--- End diff --
That's fine. We should get rid of that eyesore some time soon though,
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---