You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by ivakegg <gi...@git.apache.org> on 2016/04/03 06:59:52 UTC
[GitHub] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
GitHub user ivakegg opened a pull request:
https://github.com/apache/accumulo/pull/87
ACCUMULO-4178: Updated the sender thread to include the table ids.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ivakegg/accumulo ACCUMULO-4178
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/87.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 #87
----
commit 47d507d75826e09b4ae65c83b11b084b819a5b91
Author: Ivan Bella <iv...@bella.name>
Date: 2016-04-03T04:49:55Z
ACCUMULO-4178: Updated the sender thread to include the table ids.
----
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58382167
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
--- End diff --
When java generates code for string concatenation, it uses StringBuilder. Is there any value in this case for using StringBuilder over concatenation?
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58416898
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
+ msg.append("sending ").append(String.format("%,d", count)).append(" mutations to ").append(String.format("%,d", mutationBatch.size()))
+ .append(" tablets for ").append(String.format("%,d", tableIds.size())).append(" tables ").append(tableIds).append(" to ").append(location);
--- End diff --
TreeSet already adds []. Perhaps I should not depend on a toString method however and do the work myself to ensure it is added correctly.
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/87#issuecomment-205436724
Could put the table ids at the end of the message.
For example the following
```
sending 20 mutations to 5 tablets for [5a] to 192.168.1.103
sending 200 mutations to 10 tablets for [ab,cd,ef,gh,ij,k1,mn,rst,xyz] to 192.168.1.103
```
vs
```
sending 20 mutations to 5 tablets at 192.168.1.103 tids: [5a]
sending 200 mutations to 10 tablets at 192.168.1.103 tids: [ab,cd,ef,gh,ij,k1,mn,rst,xyz]
```
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58383515
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
+ msg.append("sending ").append(String.format("%,d", count)).append(" mutations to ").append(String.format("%,d", mutationBatch.size()))
+ .append(" tablets for ").append(String.format("%,d", tableIds.size())).append(" tables ").append(tableIds).append(" to ").append(location);
--- End diff --
Usually the toString method for collections will enclose the output with something like []. I suspect TreeSet does this.
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/87#issuecomment-205403386
Keith and Dave summed up the comments that I had to add already :)
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on the pull request:
https://github.com/apache/accumulo/pull/87#issuecomment-205819839
I think I have addresses all concerns here. Any votes for merging this in?
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58417479
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
+ msg.append("sending ").append(String.format("%,d", count)).append(" mutations to ").append(String.format("%,d", mutationBatch.size()))
+ .append(" tablets for ").append(String.format("%,d", tableIds.size())).append(" tables ").append(tableIds).append(" to ").append(location);
--- End diff --
Also I agree including the number of table ids is redundant. I will remove that part....
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/87#issuecomment-205821256
LGTM :+1:
I'll work on merging this in. Thanks, @ivakegg!
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58417086
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
--- End diff --
fair enough, but is there any downside to using it here? Especially if I am going to add the table ids myself instead of depending on the TreeSet implementation of toString()....
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on the pull request:
https://github.com/apache/accumulo/pull/87#issuecomment-205464883
> Could put the table ids at the end of the message.
I do not have a real opinion either way, but if anybody has built tools to parse this message/title then I expect putting them at the end would probably be better. I will make that change.
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58382980
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -16,37 +16,7 @@
*/
package org.apache.accumulo.core.client.impl;
-import java.io.IOException;
--- End diff --
What did you use for import manipulation? I don't think the Accumulo project has declared how imports have been organized. However I think we do try to organize imports using Eclipse's default settings inorder to cut down on noise generated by different IDEs organizing imports in different ways. Can we just drop this portion of the patch? Did the code add any imports?
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58412584
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
--- End diff --
I'm not aware of any positive effects of using a StringBuilder here. Like you said, Java is going to do that under the hood anyways.
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58374030
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
+ msg.append("sending ").append(String.format("%,d", count)).append(" mutations to ").append(String.format("%,d", mutationBatch.size()))
+ .append(" tablets for ").append(String.format("%,d", tableIds.size())).append(" tables ").append(tableIds).append(" to ").append(location);
--- End diff --
nitpick - suggest adding surrounding tableIds with [] or (). Or, remove tableIds.size() from the output if you are going to list the tables.
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58419963
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
--- End diff --
> fair enough, but is there any downside to using it here?
In my opinion it makes the code a little harder for humans to parse.
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/87#issuecomment-205831954
+1
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/87
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58418513
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -831,11 +814,16 @@ public void send(TabletServerMutations<Mutation> tsm) throws AccumuloServerExcep
try {
long count = 0;
- for (List<Mutation> list : mutationBatch.values()) {
- count += list.size();
+
+ Set<Text> tableIds = new TreeSet<Text>();
+ for (Map.Entry<KeyExtent,List<Mutation>> entry : mutationBatch.entrySet()) {
+ count += entry.getValue().size();
+ tableIds.add(entry.getKey().getTableId());
}
- String msg = "sending " + String.format("%,d", count) + " mutations to " + String.format("%,d", mutationBatch.size()) + " tablets at " + location;
- Thread.currentThread().setName(msg);
+ StringBuilder msg = new StringBuilder();
--- End diff --
If you're going to build up `msg` inside of the for loop, I think it makes sense to use the StringBuilder. Just in the current state, it didn't look like there was any benefit to the use of a StringBuilder (so it just came across as an unnecessary change).
---
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] accumulo pull request: ACCUMULO-4178: Updated the sender thread to...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/87#discussion_r58417178
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java ---
@@ -16,37 +16,7 @@
*/
package org.apache.accumulo.core.client.impl;
-import java.io.IOException;
--- End diff --
Sure, I will undo the reorganizing. Sorry about that.
---
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.
---