You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by ctubbsii <gi...@git.apache.org> on 2016/02/11 21:58:01 UTC
[GitHub] accumulo pull request: ACCUMULO-4143 Make protective copy of migra...
GitHub user ctubbsii opened a pull request:
https://github.com/apache/accumulo/pull/68
ACCUMULO-4143 Make protective copy of migrations
Makes a protective copy of the master's migrations, when requested, to prevent
ConcurrentModificationExceptions when the requestor iterates over the set at the
same time the master is updating it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ctubbsii/accumulo ACCUMULO-4143
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/68.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 #68
----
commit 7c5ab89ff70f5d25f764981e4f7c70da52bc8fef
Author: Christopher Tubbs <ct...@apache.org>
Date: 2016-02-11T20:55:16Z
ACCUMULO-4143 Make protective copy of migrations
Makes a protective copy of the master's migrations, when requested, to prevent
ConcurrentModificationExceptions when the requestor iterates over the set at the
same time the master is updating it.
----
---
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-4143 Make protective copy of migra...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/68#issuecomment-183091912
> I don't have a strong opinion about the immutable set. It was a thought i had while looking at the code.
Likewise for me. I thought I remember seeing other immutable wrappers in the Master similar to your change, but there's nothing wrong with this as-is.
---
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-4143 Make protective copy of migra...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/68#discussion_r52678587
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
> We could change the interface method name to "migrationsSnapshot()" to be clear, though, and to avoid careless future impl changes which change that semantics.
:+1: I like this general approach. I think migrations are just one place that we follow this pattern and could benefit from more descriptive method names.
---
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-4143 Make protective copy of migra...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/68#discussion_r52668001
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
Seems to put an unnecessary restriction on how other tools might use it, though. Didn't think that was necessary.
---
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-4143 Make protective copy of migra...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/68#issuecomment-183087366
looks good to me too. I don't have a strong opinion about the immutable set. It was a thought i had while looking at the 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] accumulo pull request: ACCUMULO-4143 Make protective copy of migra...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/68#discussion_r52676712
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
I was thinking a bigger risk would be that a user would accidentally introduce a RuntimeException (UnsupportedOperationException) because they tried to do something which worked against some implementations of the interface, but not others, and because wrapped immutability is not something which we can check at compile-time. It just seems an unnecessary restriction to me.
It's internal-only code, so we can change it any way we like in the future... and I think the risk you're highlighting is lower than the risk of somebody accidentally calling an unsupported method... and it's also much easier to protect against at the time the implementation changes. If it changes from a protective copy to another implementation, I would think it'd be obvious for the implementor to check to see if that breaks usage at that time.
For now, I think it makes sense to treat the interface as "snapshot of migrations", and the protective copy (vs. another implementation) matches up to that interface semantics. We could change the interface method name to "migrationsSnapshot()" to be clear, though, and to avoid careless future impl changes which change that semantics.
---
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-4143 Make protective copy of migra...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/68
---
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-4143 Make protective copy of migra...
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/68#discussion_r52667397
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
could return an immutable set
---
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-4143 Make protective copy of migra...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/68#discussion_r52669338
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
+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-4143 Make protective copy of migra...
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/68#discussion_r52682644
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
> I was thinking a bigger risk would be that a user would accidentally introduce a RuntimeException
My hope was that the runtime exception would possibly expose a bug (in addition to causing a bug). However, there are certainly cases where it would only cause problems and not expose any bugs.
> wrapped immutability is not something which we can check at compile-time
That would be nice to have.
---
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-4143 Make protective copy of migra...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/68#discussion_r52684501
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
I'll go ahead and do the immutability, as well as rename the method... but I'll add a Javadoc to clearly state that it's immutible. It looks like the other (test) implementations are already immutable 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] accumulo pull request: ACCUMULO-4143 Make protective copy of migra...
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/68#discussion_r52669078
--- Diff: server/master/src/main/java/org/apache/accumulo/master/Master.java ---
@@ -1336,7 +1332,11 @@ public void assignedTablet(KeyExtent extent) {
}
@Override
- public Collection<KeyExtent> migrations() {
- return migrations.keySet();
+ public Set<KeyExtent> migrations() {
+ Set<KeyExtent> migrationsCopy = new HashSet<KeyExtent>();
+ synchronized (migrations) {
+ migrationsCopy.addAll(migrations.keySet());
+ }
+ return migrationsCopy;
--- End diff --
My thinking is that we don't expect the caller to modify it. Now that a copy is made, its ok if caller modifies. But if in the future the implementation uses a concurrent map and does not copy, would not want caller of method to modify. Also the previous implementation didn't expect caller to modify, just read.
---
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.
---