You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by vrozov <gi...@git.apache.org> on 2015/11/20 00:47:06 UTC
[GitHub] incubator-apex-core pull request: APEX-273 - Fix existing checksty...
GitHub user vrozov opened a pull request:
https://github.com/apache/incubator-apex-core/pull/175
APEX-273 - Fix existing checkstyle violations in bufferserver module
@243826, @davidyan74, @tweise, @chandnisingh Please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vrozov/incubator-apex-core APEX-273
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-apex-core/pull/175.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 #175
----
commit 8223b6aa75e0a2cc39c074ca8cbde5ba55ba1570
Author: MalharJenkins <je...@datatorrent.com>
Date: 2015-11-19T23:44:13Z
APEX-273 - Fix existing checkstyle violations in bufferserver module
----
---
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-273 - Fix existing checksty...
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/175#discussion_r45421265
--- Diff: bufferserver/src/test/java/com/datatorrent/bufferserver/util/CodecTest.java ---
@@ -22,8 +22,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.annotations.Test;
-import static org.testng.Assert.*;
+import static org.junit.Assert.assertEquals;
--- End diff --
We do allow static import and use it in other classes (mostly in tests).
Bufferserver uses testng JUnit provider and it should be part of another exercise to migrate Bufferserver to org.junit from org.testng.
I will fix static import to org.testng.Assert.assertEquals.
---
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-273 - Fix existing checksty...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158236687
Do we need to do the checkstyle separately now. Wasn't there a plan to do it as needed when folks are touching relavant code.
---
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-273 - Fix existing checksty...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-apex-core/pull/175
---
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-273 - Fix existing checksty...
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/175#discussion_r45419650
--- Diff: bufferserver/src/test/java/com/datatorrent/bufferserver/util/CodecTest.java ---
@@ -22,8 +22,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.annotations.Test;
-import static org.testng.Assert.*;
+import static org.junit.Assert.assertEquals;
--- End diff --
Not sure if we allow import static. Should we just import org.junit.Assert and call Assert.assertEquals?
---
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-273 - Fix existing checksty...
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/175#discussion_r46153265
--- Diff: bufferserver/src/main/java/com/datatorrent/bufferserver/internal/DataList.java ---
@@ -139,13 +140,16 @@ public void rewind(final int baseSeconds, final int windowId) throws IOException
}
/*
- TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last block.
- */
+ * TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last
+ * block.
+ */
final int numberOfInMemBlockPermits = this.numberOfInMemBlockPermits.addAndGet(numberOfInMemBlockRewound);
- assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " + numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
+ assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " +
+ numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
resumeSuspendedClients(numberOfInMemBlockPermits);
- logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after rewinding {}. ", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
+ logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after" +
+ " rewinding {}.", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
--- End diff --
Not sure what we can do with the string literals that exceed 120 chars.
---
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-273 - Fix existing checksty...
Posted by 243826 <gi...@git.apache.org>.
Github user 243826 commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158282682
when did the voting happen on this issue?
On Thu, Nov 19, 2015 at 3:55 PM, Vlad Rozov <no...@github.com>
wrote:
> This option lost in the community vote and I did not see any new vote, so
> I believe the plan is the same - take the pain and fix all code style
> violations.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158237603>
> .
>
---
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-273 - Fix existing checksty...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158238032
If the vote is to change it, can we use a special account like release to be the author of the changes so we can flag these as codestyle changes in future.
---
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-273 - Fix existing checksty...
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/175#discussion_r45419757
--- Diff: bufferserver/src/test/java/com/datatorrent/bufferserver/util/CodecTest.java ---
@@ -22,8 +22,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.annotations.Test;
-import static org.testng.Assert.*;
+import static org.junit.Assert.assertEquals;
--- End diff --
Also, should we get rid of all org.testing imports and use org.junit like the rest of the Apex core?
---
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-273 - Fix existing checksty...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158237603
This option lost in the community vote and I did not see any new vote, so I believe the plan is the same - take the pain and fix all code style 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-273 - Fix existing checksty...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/175#discussion_r46100090
--- Diff: bufferserver/src/main/java/com/datatorrent/bufferserver/internal/DataList.java ---
@@ -139,13 +140,16 @@ public void rewind(final int baseSeconds, final int windowId) throws IOException
}
/*
- TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last block.
- */
+ * TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last
+ * block.
+ */
final int numberOfInMemBlockPermits = this.numberOfInMemBlockPermits.addAndGet(numberOfInMemBlockRewound);
- assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " + numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
+ assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " +
+ numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
resumeSuspendedClients(numberOfInMemBlockPermits);
- logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after rewinding {}. ", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
+ logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after" +
+ " rewinding {}.", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
--- End diff --
This change set illustrates in several cases how readability is affected by the line length enforcement. Breaking string literals to make checkstyle happy looks just wrong.
---
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-273 - Fix existing checksty...
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/175#discussion_r46191846
--- Diff: bufferserver/src/main/java/com/datatorrent/bufferserver/internal/DataList.java ---
@@ -139,13 +140,16 @@ public void rewind(final int baseSeconds, final int windowId) throws IOException
}
/*
- TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last block.
- */
+ * TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last
+ * block.
+ */
final int numberOfInMemBlockPermits = this.numberOfInMemBlockPermits.addAndGet(numberOfInMemBlockRewound);
- assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " + numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
+ assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " +
+ numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
resumeSuspendedClients(numberOfInMemBlockPermits);
- logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after rewinding {}. ", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
+ logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after" +
+ " rewinding {}.", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
--- End diff --
I agree with Thomas on this. I think in this case, we should break it after the comma and not break the string literal, even if the string literal makes the line over 120 chars.
---
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-273 - Fix existing checksty...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158240043
Do we need a new special account? Will it be sufficient to use MalharJenkins?
---
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-273 - Fix existing checksty...
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/175#discussion_r46152484
--- Diff: bufferserver/src/main/java/com/datatorrent/bufferserver/internal/DataList.java ---
@@ -177,11 +181,13 @@ public void reset()
public void purge(final int baseSeconds, final int windowId)
{
final long longWindowId = (long)baseSeconds << 32 | windowId;
- logger.debug("Purging {} from window ID {} to window ID {}", this, Codec.getStringWindowId(first.starting_window), Codec.getStringWindowId(longWindowId));
+ logger.debug("Purging {} from window ID {} to window ID {}", this, Codec.getStringWindowId(first.starting_window),
+ Codec.getStringWindowId(longWindowId));
int numberOfInMemBlockPurged = 0;
synchronized (this) {
- for (Block prev = null, temp = first; temp != null && temp.starting_window <= longWindowId; prev = temp, temp = temp.next) {
+ for (Block prev = null, temp = first; temp != null && temp.starting_window <= longWindowId;
--- End diff --
It depends on the width of the monitor. When I don't use external monitor, wrapping at 120 char length improves readability.
---
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-273 - Fix existing checksty...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/175#discussion_r46100313
--- Diff: bufferserver/src/test/java/com/datatorrent/bufferserver/server/ServerTest.java ---
@@ -72,7 +72,8 @@ public static void setupServerAndClients() throws Exception
instance = new Server(0, 4096,8);
address = instance.run(eventloopServer);
- assert (address instanceof InetSocketAddress);
+ assertTrue(address instanceof InetSocketAddress);
+ assertFalse(address.isUnresolved());
--- End diff --
Why was there code for dealing with unresolved address before?
---
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-273 - Fix existing checksty...
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/175#discussion_r46201146
--- Diff: bufferserver/src/main/java/com/datatorrent/bufferserver/internal/DataList.java ---
@@ -139,13 +140,16 @@ public void rewind(final int baseSeconds, final int windowId) throws IOException
}
/*
- TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last block.
- */
+ * TODO: properly rewind Data List iterators, especially handle case when iterators point to blocks past the last
+ * block.
+ */
final int numberOfInMemBlockPermits = this.numberOfInMemBlockPermits.addAndGet(numberOfInMemBlockRewound);
- assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " + numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
+ assert numberOfInMemBlockPermits < MAX_COUNT_OF_INMEM_BLOCKS : "Number of in memory block permits " +
+ numberOfInMemBlockPermits + " exceeded configured maximum " + MAX_COUNT_OF_INMEM_BLOCKS + '.';
resumeSuspendedClients(numberOfInMemBlockPermits);
- logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after rewinding {}. ", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
+ logger.debug("Discarded {} in memory blocks during rewind. Number of in memory blocks permits {} after" +
+ " rewinding {}.", numberOfInMemBlockRewound, numberOfInMemBlockPermits, this);
--- End diff --
I disagree that we should allow exceptions to code style rules. Either 120 line length needs to be enforced or it should not be enforced at all, not just for string literals. Note that some string literals may be very long and span multiple lines.
---
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-273 - Fix existing checksty...
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/175#discussion_r46152198
--- Diff: bufferserver/src/test/java/com/datatorrent/bufferserver/server/ServerTest.java ---
@@ -72,7 +72,8 @@ public static void setupServerAndClients() throws Exception
instance = new Server(0, 4096,8);
address = instance.run(eventloopServer);
- assert (address instanceof InetSocketAddress);
+ assertTrue(address instanceof InetSocketAddress);
+ assertFalse(address.isUnresolved());
--- End diff --
I think assert for InetSocketAddress (address is declared as InetSocketAddress) and dealing with unresolved address were parts of the original code that was later refactored. Server.run() returns resolved InetSocketAddress.
---
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-273 - Fix existing checksty...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/175#discussion_r46100108
--- Diff: bufferserver/src/main/java/com/datatorrent/bufferserver/internal/DataList.java ---
@@ -177,11 +181,13 @@ public void reset()
public void purge(final int baseSeconds, final int windowId)
{
final long longWindowId = (long)baseSeconds << 32 | windowId;
- logger.debug("Purging {} from window ID {} to window ID {}", this, Codec.getStringWindowId(first.starting_window), Codec.getStringWindowId(longWindowId));
+ logger.debug("Purging {} from window ID {} to window ID {}", this, Codec.getStringWindowId(first.starting_window),
+ Codec.getStringWindowId(longWindowId));
int numberOfInMemBlockPurged = 0;
synchronized (this) {
- for (Block prev = null, temp = first; temp != null && temp.starting_window <= longWindowId; prev = temp, temp = temp.next) {
+ for (Block prev = null, temp = first; temp != null && temp.starting_window <= longWindowId;
--- End diff --
Don't see the improvement in this case either, line was 130 chars long before.
---
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-273 - Fix existing checksty...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/175#issuecomment-158240218
MalharJenkins will do
---
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.
---