[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

classic Classic list List threaded Threaded
110 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
GitHub user ivakegg opened a pull request:

    https://github.com/apache/accumulo/pull/260

    ACCUMULO-4643 initial implementation

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ivakegg/accumulo ACCUMULO-4643

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/260.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 #260
   
----
commit 8100aacd8f79d6b670e65505a272b303b552750f
Author: Ivan Bella <[hidden email]>
Date:   2017-05-25T18:22:24Z

    ACCUMULO-4643 initial implementation

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #260: ACCUMULO-4643 initial implementation

ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/260
 
    still working the test cases, but this code should show you the idea....


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118568742
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/YieldScannersIT.java ---
    @@ -0,0 +1,71 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test;
    +
    +import com.google.common.collect.Iterators;
    +import org.apache.accumulo.core.client.Connector;
    +import org.apache.accumulo.core.client.IteratorSetting;
    +import org.apache.accumulo.core.client.Scanner;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.security.Authorizations;
    +import org.apache.accumulo.harness.AccumuloClusterHarness;
    +import org.apache.accumulo.minicluster.impl.MiniAccumuloConfigImpl;
    +import org.apache.accumulo.test.functional.YieldingIterator;
    +import org.apache.hadoop.conf.Configuration;
    +import org.junit.Assert;
    +import org.junit.Test;
    +
    +import java.util.Iterator;
    +import java.util.Map;
    +
    +// ACCUMULO-4643
    +public class YieldScannersIT extends AccumuloClusterHarness {
    +
    +  @Override
    +  public int defaultTimeoutSeconds() {
    +    return 600000;
    --- End diff --
   
    Seems really long to read 10 entries :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118567373
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/ScanYieldException.java ---
    @@ -0,0 +1,35 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.iterators.user;
    +
    +import org.apache.accumulo.core.data.Key;
    +
    +/**
    + * This exception can be thrown at from a next or seek call on an iterator to allow other scans to get time slots. This mechanism is intended to avoid a set of
    --- End diff --
   
    Let's also add a note to `SortedKeyValueIterator#next()` and `SortedKeyValueIterator#seek(...)`.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118567242
 
    --- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
    @@ -548,11 +549,13 @@ private LookupResult lookup(SortedKeyValueIterator<Key,Value> mmfi, List<Range>
           batchTimeOut = 0;
         }
     
    +    boolean yield = false;
    +
         for (Range range : ranges) {
     
           boolean timesUp = batchTimeOut > 0 && System.nanoTime() > returnTime;
     
    -      if (exceededMemoryUsage || tabletClosed || timesUp) {
    +      if (exceededMemoryUsage || tabletClosed || timesUp || yield) {
             lookupResult.unfinishedRanges.add(range);
             continue;
    --- End diff --
   
    The way the Exception's javadoc reads, it would preempty the entire Scan RPC, but it's actually just pre-empting one Range. Is that intentional?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118568041
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/YieldScannersIT.java ---
    @@ -0,0 +1,71 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test;
    +
    +import com.google.common.collect.Iterators;
    +import org.apache.accumulo.core.client.Connector;
    +import org.apache.accumulo.core.client.IteratorSetting;
    +import org.apache.accumulo.core.client.Scanner;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.security.Authorizations;
    +import org.apache.accumulo.harness.AccumuloClusterHarness;
    +import org.apache.accumulo.minicluster.impl.MiniAccumuloConfigImpl;
    +import org.apache.accumulo.test.functional.YieldingIterator;
    +import org.apache.hadoop.conf.Configuration;
    +import org.junit.Assert;
    +import org.junit.Test;
    +
    +import java.util.Iterator;
    +import java.util.Map;
    +
    +// ACCUMULO-4643
    +public class YieldScannersIT extends AccumuloClusterHarness {
    +
    +  @Override
    +  public int defaultTimeoutSeconds() {
    +    return 600000;
    +  }
    +
    +  @Override
    +  public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) {
    +    cfg.setNumTservers(1);
    +  }
    +
    +  @Test
    +  public void test() throws Exception {
    +    // make a table
    +    final String tableName = getUniqueNames(1)[0];
    +    final Connector conn = getConnector();
    +    conn.tableOperations().create(tableName);
    +
    +    // make a scanner for a table with 10 keys
    +    final Scanner scanner = conn.createScanner(tableName, Authorizations.EMPTY);
    +    final IteratorSetting cfg = new IteratorSetting(100, YieldingIterator.class);
    +    cfg.addOption(YieldingIterator.NUMBER_OF_KEYS, "10");
    --- End diff --
   
    What about just writing 10 entries to the table and then using your iterator to read them all.
   
    IIUC, the client should see no difference with this change in place (just the number of RPCs changes).


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118569076
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/YieldScannersIT.java ---
    @@ -0,0 +1,71 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test;
    +
    +import com.google.common.collect.Iterators;
    +import org.apache.accumulo.core.client.Connector;
    +import org.apache.accumulo.core.client.IteratorSetting;
    +import org.apache.accumulo.core.client.Scanner;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.security.Authorizations;
    +import org.apache.accumulo.harness.AccumuloClusterHarness;
    +import org.apache.accumulo.minicluster.impl.MiniAccumuloConfigImpl;
    +import org.apache.accumulo.test.functional.YieldingIterator;
    +import org.apache.hadoop.conf.Configuration;
    +import org.junit.Assert;
    +import org.junit.Test;
    +
    +import java.util.Iterator;
    +import java.util.Map;
    +
    +// ACCUMULO-4643
    +public class YieldScannersIT extends AccumuloClusterHarness {
    +
    +  @Override
    +  public int defaultTimeoutSeconds() {
    +    return 600000;
    +  }
    +
    +  @Override
    +  public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) {
    +    cfg.setNumTservers(1);
    +  }
    +
    +  @Test
    +  public void test() throws Exception {
    +    // make a table
    +    final String tableName = getUniqueNames(1)[0];
    +    final Connector conn = getConnector();
    +    conn.tableOperations().create(tableName);
    +
    +    // make a scanner for a table with 10 keys
    +    final Scanner scanner = conn.createScanner(tableName, Authorizations.EMPTY);
    +    final IteratorSetting cfg = new IteratorSetting(100, YieldingIterator.class);
    +    cfg.addOption(YieldingIterator.NUMBER_OF_KEYS, "10");
    --- End diff --
   
    Sorry, saw your second note about tweaking the test after leaving 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/260
 
    I recommend improving the log message before this gets merged. "initial implementation" doesn't quite explain what it does. The message has a reference to a JIRA, but a brief description within the git history is still nice.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118574189
 
    --- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
    @@ -548,11 +549,13 @@ private LookupResult lookup(SortedKeyValueIterator<Key,Value> mmfi, List<Range>
           batchTimeOut = 0;
         }
     
    +    boolean yield = false;
    +
         for (Range range : ranges) {
     
           boolean timesUp = batchTimeOut > 0 && System.nanoTime() > returnTime;
     
    -      if (exceededMemoryUsage || tabletClosed || timesUp) {
    +      if (exceededMemoryUsage || tabletClosed || timesUp || yield) {
             lookupResult.unfinishedRanges.add(range);
             continue;
    --- End diff --
   
    yes, it interrupts only the range currently being scanned.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118574263
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/ScanYieldException.java ---
    @@ -0,0 +1,35 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.iterators.user;
    +
    +import org.apache.accumulo.core.data.Key;
    +
    +/**
    + * This exception can be thrown at from a next or seek call on an iterator to allow other scans to get time slots. This mechanism is intended to avoid a set of
    --- End diff --
   
    Agreed, 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118574368
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/YieldScannersIT.java ---
    @@ -0,0 +1,71 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test;
    +
    +import com.google.common.collect.Iterators;
    +import org.apache.accumulo.core.client.Connector;
    +import org.apache.accumulo.core.client.IteratorSetting;
    +import org.apache.accumulo.core.client.Scanner;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.security.Authorizations;
    +import org.apache.accumulo.harness.AccumuloClusterHarness;
    +import org.apache.accumulo.minicluster.impl.MiniAccumuloConfigImpl;
    +import org.apache.accumulo.test.functional.YieldingIterator;
    +import org.apache.hadoop.conf.Configuration;
    +import org.junit.Assert;
    +import org.junit.Test;
    +
    +import java.util.Iterator;
    +import java.util.Map;
    +
    +// ACCUMULO-4643
    +public class YieldScannersIT extends AccumuloClusterHarness {
    +
    +  @Override
    +  public int defaultTimeoutSeconds() {
    +    return 600000;
    --- End diff --
   
    right, I was having issues with Mini Accumulo on my box.  I will set that appropriately.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r118574720
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/YieldScannersIT.java ---
    @@ -0,0 +1,71 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test;
    +
    +import com.google.common.collect.Iterators;
    +import org.apache.accumulo.core.client.Connector;
    +import org.apache.accumulo.core.client.IteratorSetting;
    +import org.apache.accumulo.core.client.Scanner;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.security.Authorizations;
    +import org.apache.accumulo.harness.AccumuloClusterHarness;
    +import org.apache.accumulo.minicluster.impl.MiniAccumuloConfigImpl;
    +import org.apache.accumulo.test.functional.YieldingIterator;
    +import org.apache.hadoop.conf.Configuration;
    +import org.junit.Assert;
    +import org.junit.Test;
    +
    +import java.util.Iterator;
    +import java.util.Map;
    +
    +// ACCUMULO-4643
    +public class YieldScannersIT extends AccumuloClusterHarness {
    +
    +  @Override
    +  public int defaultTimeoutSeconds() {
    +    return 600000;
    +  }
    +
    +  @Override
    +  public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) {
    +    cfg.setNumTservers(1);
    +  }
    +
    +  @Test
    +  public void test() throws Exception {
    +    // make a table
    +    final String tableName = getUniqueNames(1)[0];
    +    final Connector conn = getConnector();
    +    conn.tableOperations().create(tableName);
    +
    +    // make a scanner for a table with 10 keys
    +    final Scanner scanner = conn.createScanner(tableName, Authorizations.EMPTY);
    +    final IteratorSetting cfg = new IteratorSetting(100, YieldingIterator.class);
    +    cfg.addOption(YieldingIterator.NUMBER_OF_KEYS, "10");
    --- End diff --
   
    I could write the entries to the table, but the iterator I am using simulates that just fine.  I will adjust the test case 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/260
 
    I shall rebase and rewrite the message.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/260
 
    I am pretty satisfied this is now working as designed.  The test case uses an iterator that will throw a yield exception every other next and every other seek.  The test case also detects when the iterator has been torn down and rebuilt.  From the client perspective, nothing changes (all keys returned appropriately).


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119114135
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/functional/YieldingIterator.java ---
    @@ -0,0 +1,88 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test.functional;
    +
    +import java.util.concurrent.atomic.AtomicBoolean;
    +import org.apache.accumulo.core.data.ByteSequence;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Range;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.iterators.IteratorEnvironment;
    +import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
    +import org.apache.accumulo.core.iterators.WrappingIterator;
    +import org.apache.accumulo.core.iterators.user.ScanYieldException;
    +
    +import java.io.IOException;
    +import java.util.Collection;
    +import java.util.concurrent.atomic.AtomicInteger;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +/**
    + * This iterator will throw a ScanYieldException every other key
    + */
    +public class YieldingIterator extends WrappingIterator {
    +  private static final Logger log = LoggerFactory.getLogger(YieldingIterator.class);
    +  private static final AtomicInteger yieldNexts = new AtomicInteger(0);
    +  private static final AtomicInteger yieldSeeks = new AtomicInteger(0);
    +  private static final AtomicInteger rebuilds = new AtomicInteger(0);
    +
    +  private static final AtomicBoolean yieldNextKey = new AtomicBoolean(false);
    +  private static final AtomicBoolean yieldSeekKey = new AtomicBoolean(false);
    +
    +  @Override
    +  public SortedKeyValueIterator<Key,Value> deepCopy(IteratorEnvironment env) {
    +    YieldingIterator it = new YieldingIterator();
    +    it.setSource(it.getSource().deepCopy(env));
    +    return it;
    +  }
    +
    +  @Override
    +  public void next() throws IOException {
    +    log.info("start YieldingIterator.next: " + getTopValue());
    +    yieldNextKey.set(!yieldNextKey.get());
    +    if (yieldNextKey.get()) {
    +      log.info("YieldingIterator.next yielding: " + getTopValue());
    +      yieldNexts.incrementAndGet();
    +      throw new ScanYieldException(getTopKey());
    +    }
    +    super.next();
    +    log.info("end YieldingIterator.next: " + (hasTop() ? getTopKey() + " " + getTopValue() : "no top"));
    +  }
    +
    +  @Override
    +  public Value getTopValue() {
    +    String value = Integer.toString(yieldNexts.get()) + ',' + Integer.toString(yieldSeeks.get()) + ',' + Integer.toString(rebuilds.get());
    +    return new Value(value);
    +  }
    +
    +  @Override
    +  public void seek(Range range, Collection<ByteSequence> columnFamilies, boolean inclusive) throws IOException {
    +    log.info("start YieldingIterator.seek: " + getTopValue() + " with range " + range);
    --- End diff --
   
    shouldn't this be log.info("start YieldingIterator.seek: {} with range {}",getTopValue(),range); ?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119114193
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/functional/YieldingIterator.java ---
    @@ -0,0 +1,88 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test.functional;
    +
    +import java.util.concurrent.atomic.AtomicBoolean;
    +import org.apache.accumulo.core.data.ByteSequence;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Range;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.iterators.IteratorEnvironment;
    +import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
    +import org.apache.accumulo.core.iterators.WrappingIterator;
    +import org.apache.accumulo.core.iterators.user.ScanYieldException;
    +
    +import java.io.IOException;
    +import java.util.Collection;
    +import java.util.concurrent.atomic.AtomicInteger;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +/**
    + * This iterator will throw a ScanYieldException every other key
    + */
    +public class YieldingIterator extends WrappingIterator {
    +  private static final Logger log = LoggerFactory.getLogger(YieldingIterator.class);
    +  private static final AtomicInteger yieldNexts = new AtomicInteger(0);
    +  private static final AtomicInteger yieldSeeks = new AtomicInteger(0);
    +  private static final AtomicInteger rebuilds = new AtomicInteger(0);
    +
    +  private static final AtomicBoolean yieldNextKey = new AtomicBoolean(false);
    +  private static final AtomicBoolean yieldSeekKey = new AtomicBoolean(false);
    +
    +  @Override
    +  public SortedKeyValueIterator<Key,Value> deepCopy(IteratorEnvironment env) {
    +    YieldingIterator it = new YieldingIterator();
    +    it.setSource(it.getSource().deepCopy(env));
    +    return it;
    +  }
    +
    +  @Override
    +  public void next() throws IOException {
    +    log.info("start YieldingIterator.next: " + getTopValue());
    +    yieldNextKey.set(!yieldNextKey.get());
    +    if (yieldNextKey.get()) {
    +      log.info("YieldingIterator.next yielding: " + getTopValue());
    +      yieldNexts.incrementAndGet();
    +      throw new ScanYieldException(getTopKey());
    +    }
    +    super.next();
    +    log.info("end YieldingIterator.next: " + (hasTop() ? getTopKey() + " " + getTopValue() : "no top"));
    +  }
    +
    +  @Override
    +  public Value getTopValue() {
    +    String value = Integer.toString(yieldNexts.get()) + ',' + Integer.toString(yieldSeeks.get()) + ',' + Integer.toString(rebuilds.get());
    --- End diff --
   
    This can only be used where values are not 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119107638
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/iterators/SortedKeyValueIterator.java ---
    @@ -69,6 +70,10 @@
        *              if called before seek.
        * @exception NoSuchElementException
        *              if next element doesn't exist.
    +   * @exception ScanYieldException
    +   *              Thrown if the iterator is permitting the scan to be torn down to allow other scans to use this thread. An iterator may throw this if it had
    --- End diff --
   
    s/if it had/if it has/


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119115828
 
    --- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
    @@ -586,6 +589,10 @@ private LookupResult lookup(SortedKeyValueIterator<Key,Value> mmfi, List<Range>
               mmfi.next();
             }
     
    +      } catch (ScanYieldException sye) {
    +        log.debug("Scan yield exception detected at position " + sye.getPosition());
    +        addUnfinishedRange(lookupResult, range, sye.getPosition(), false);
    --- End diff --
   
    What happens to data which is buffered to be sent back to the client, but has not yet been sent? (the buffer controlled by the tserver scan max memory property). I think the client would see this data, but I wanted to dbl check with you as you're more familiar than I am now :)
   
    Specifically, say we have a buffer which can hold 500 Key-Values. An Iterator gets through 250 K-V's, and then throws this exception. We mark the `Range` as unfinished, but does the client see those 250 K-V's that were produced by the Iterator?
   
    It seems like `addUnfinishedRange(..)` is then determining for us if we need to do any more work on the current range (e.g. handling the case where we "timed out" after computing the last k-v for the Range).


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119120650
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/ScanYieldException.java ---
    @@ -0,0 +1,35 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.iterators.user;
    +
    +import org.apache.accumulo.core.data.Key;
    +
    +/**
    + * This exception can be thrown at from a next or seek call on an iterator to allow other scans to get time slots. This mechanism is intended to avoid a set of
    + * scans to dominate all of the scan slots (readahead threads) and starve other scans out.
    + */
    +public class ScanYieldException extends RuntimeException {
    +  private final Key position;
    +
    +  public ScanYieldException(Key position) {
    --- End diff --
   
    Can we make this more specific as to what `Key` should be passed in? If I understand correctly:
   
    1) For seek(), the start key of the Range passed to the Iterator
    2) For next(), the last key returned by the Iterator
   
    Maybe there is a third case for when seek() also consumes data before returning the call (finding the first key in the range which matches some condition)? I'm not sure of a concise way to state 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation

ctubbsii
In reply to this post by ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119116363
 
    --- Diff: test/src/main/java/org/apache/accumulo/test/YieldScannersIT.java ---
    @@ -0,0 +1,106 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.test;
    +
    +import org.apache.accumulo.core.client.BatchWriter;
    +import org.apache.accumulo.core.client.BatchWriterConfig;
    +import org.apache.accumulo.core.client.Connector;
    +import org.apache.accumulo.core.client.IteratorSetting;
    +import org.apache.accumulo.core.client.Scanner;
    +import org.apache.accumulo.core.data.Key;
    +import org.apache.accumulo.core.data.Mutation;
    +import org.apache.accumulo.core.data.Value;
    +import org.apache.accumulo.core.security.Authorizations;
    +import org.apache.accumulo.harness.AccumuloClusterHarness;
    +import org.apache.accumulo.minicluster.impl.MiniAccumuloConfigImpl;
    +import org.apache.accumulo.test.functional.YieldingIterator;
    +import org.apache.commons.lang.StringUtils;
    +import org.apache.hadoop.conf.Configuration;
    +import org.apache.hadoop.io.Text;
    +import org.junit.Assert;
    +import org.junit.Test;
    +
    +import java.util.Iterator;
    +import java.util.Map;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +// ACCUMULO-4643
    +public class YieldScannersIT extends AccumuloClusterHarness {
    +  Logger log = LoggerFactory.getLogger(YieldScannersIT.class);
    +
    +  @Override
    +  public int defaultTimeoutSeconds() {
    +    return 60;
    +  }
    +
    +  @Override
    +  public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) {
    +    cfg.setNumTservers(1);
    +  }
    +
    +  @Test
    +  public void test() throws Exception {
    +    // make a table
    +    final String tableName = getUniqueNames(1)[0];
    +    final Connector conn = getConnector();
    +    conn.tableOperations().create(tableName);
    +    final BatchWriter writer = conn.createBatchWriter(tableName, new BatchWriterConfig());
    +    for (int i = 0; i < 10; i++) {
    +      byte[] row = new byte[] {(byte) ('a' + i)};
    +      Mutation m = new Mutation(new Text(row));
    +      m.put(new Text(), new Text(), new Value());
    +      writer.addMutation(m);
    +    }
    +    writer.flush();
    +    writer.close();
    +
    +    log.info("Creating scanner");
    +    // make a scanner for a table with 10 keys
    +    final Scanner scanner = conn.createScanner(tableName, Authorizations.EMPTY);
    +    final IteratorSetting cfg = new IteratorSetting(100, YieldingIterator.class);
    +    scanner.addScanIterator(cfg);
    +
    +    log.info("iterating");
    +    Iterator<Map.Entry<Key,Value>> it = scanner.iterator();
    +    int keyCount = 0;
    +    int yieldNextCount = 0;
    +    int yieldSeekCount = 0;
    +    while (it.hasNext()) {
    +      Map.Entry<Key,Value> next = it.next();
    +      log.info(Integer.toString(keyCount) + ": Got key " + next.getKey() + " with value " + next.getValue());
    +
    +      // verify we got the expected key
    +      char expected = (char) ('a' + keyCount);
    --- End diff --
   
    How about we just use the code point for the lowercase letter 'a' instead of an actual `'a'`? Took me a moment to realize what you were doing :)


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
1234 ... 6
Loading...