accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubbsii <...@git.apache.org>
Subject [GitHub] accumulo pull request: ACCUMULO-4143 Make protective copy of migra...
Date Thu, 11 Feb 2016 22:17:15 GMT
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.
---

Mime
View raw message