[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

classic Classic list List threaded Threaded
81 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

ctubbsii
GitHub user dlmarion opened a pull request:

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

    ACCUMULO-4463: Make block cache implentation configurable

   

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

    $ git pull https://github.com/apache/accumulo ACCUMULO-4463

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

    https://github.com/apache/accumulo/pull/256.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 #256
   
----
commit 03a8f2acfb4f95b545c47a4e681dd5b8fa585c94
Author: Dave Marion <[hidden email]>
Date:   2017-05-08T15:47:53Z

    ACCUMULO-4463: Make block cache implentation configurable

----


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

ctubbsii
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/256#discussion_r115284646
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -245,7 +245,8 @@
       TSERV_PREFIX("tserver.", null, PropertyType.PREFIX, "Properties in this category affect the behavior of the tablet servers"),
       TSERV_CLIENT_TIMEOUT("tserver.client.timeout", "3s", PropertyType.TIMEDURATION, "Time to wait for clients to continue scans before closing a session."),
       TSERV_DEFAULT_BLOCKSIZE("tserver.default.blocksize", "1M", PropertyType.BYTES, "Specifies a default blocksize for the tserver caches"),
    -  TSERV_CACHE_POLICY("tserver.cache.policy", "LRU", PropertyType.STRING, "Specifies the eviction policy of the file data caches (LRU or TinyLFU)."),
    --- End diff --
   
    Need to support backwards compatibility for configuration properties.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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/256#discussion_r115286028
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java ---
    @@ -17,10 +17,30 @@
      */
     package org.apache.accumulo.core.file.blockfile.cache;
     
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
    +
     /**
      * Block cache interface.
      */
     public interface BlockCache {
    +
    +  /**
    +   * Start the block cache
    +   *
    +   * @param conf
    +   *          Accumulo configuration object
    +   * @param maxSize
    +   *          maximum size of the on-heap cache
    +   * @param blockSize
    +   *          size of the default RFile blocks
    +   */
    +  void start(AccumuloConfiguration conf, long maxSize, long blockSize) throws Exception;
    --- End diff --
   
    Also, we could separate the configuration and start logic into separate methods.
   
    Unrelated thought: what about creating a POJO to encapsulate this information. It would prevent drift in the configuration details from breaking blockcache impls (e.g. BlockCacheConfiguration, presently made up of an AccuConf, maxSize, and blockSize).


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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/256#discussion_r115285384
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java ---
    @@ -17,10 +17,30 @@
      */
     package org.apache.accumulo.core.file.blockfile.cache;
     
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
    +
     /**
      * Block cache interface.
      */
     public interface BlockCache {
    +
    +  /**
    +   * Start the block cache
    +   *
    +   * @param conf
    +   *          Accumulo configuration object
    +   * @param maxSize
    +   *          maximum size of the on-heap cache
    +   * @param blockSize
    +   *          size of the default RFile blocks
    +   */
    +  void start(AccumuloConfiguration conf, long maxSize, long blockSize) throws Exception;
    --- End diff --
   
    If `Exception` is just being caught and re-thrown as RTE, isn't it cleaner to just removes the `throws` here and inform, via javadoc, implementations should throw an RTE if they cannot start successfully?


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115287907
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -245,7 +245,8 @@
       TSERV_PREFIX("tserver.", null, PropertyType.PREFIX, "Properties in this category affect the behavior of the tablet servers"),
       TSERV_CLIENT_TIMEOUT("tserver.client.timeout", "3s", PropertyType.TIMEDURATION, "Time to wait for clients to continue scans before closing a session."),
       TSERV_DEFAULT_BLOCKSIZE("tserver.default.blocksize", "1M", PropertyType.BYTES, "Specifies a default blocksize for the tserver caches"),
    -  TSERV_CACHE_POLICY("tserver.cache.policy", "LRU", PropertyType.STRING, "Specifies the eviction policy of the file data caches (LRU or TinyLFU)."),
    --- End diff --
   
    for a 2.0 release?


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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/256#discussion_r115288481
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -245,7 +245,8 @@
       TSERV_PREFIX("tserver.", null, PropertyType.PREFIX, "Properties in this category affect the behavior of the tablet servers"),
       TSERV_CLIENT_TIMEOUT("tserver.client.timeout", "3s", PropertyType.TIMEDURATION, "Time to wait for clients to continue scans before closing a session."),
       TSERV_DEFAULT_BLOCKSIZE("tserver.default.blocksize", "1M", PropertyType.BYTES, "Specifies a default blocksize for the tserver caches"),
    -  TSERV_CACHE_POLICY("tserver.cache.policy", "LRU", PropertyType.STRING, "Specifies the eviction policy of the file data caches (LRU or TinyLFU)."),
    --- End diff --
   
    Yes.
   
    And, before we get there, I am aware that this is technically not covered by our semver use. However, historically, we have treated configuration properties as such. We should not be dropping old, user-defined configuration keys without a deprecation cycle.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115288967
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java ---
    @@ -17,10 +17,30 @@
      */
     package org.apache.accumulo.core.file.blockfile.cache;
     
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
    +
     /**
      * Block cache interface.
      */
     public interface BlockCache {
    +
    +  /**
    +   * Start the block cache
    +   *
    +   * @param conf
    +   *          Accumulo configuration object
    +   * @param maxSize
    +   *          maximum size of the on-heap cache
    +   * @param blockSize
    +   *          size of the default RFile blocks
    +   */
    +  void start(AccumuloConfiguration conf, long maxSize, long blockSize) throws Exception;
    --- End diff --
   
    re: POJO -> I was going to just use the AccumuloConfiguration and the custom properties for this. I'm not sure how a single BlockCacheConfiguration class would work across different BlockCache impls that have different configuration parameters.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115290117
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java ---
    @@ -84,7 +85,7 @@
       static final int statThreadPeriod = 60;
     
       /** Concurrent map (the cache) */
    -  private final ConcurrentHashMap<String,CachedBlock> map;
    --- End diff --
   
    It may be nice to keep the ability to have these final for performance reasons.  I think we could do this if we made the BlockCacheFactory configurable instead of the BlockCache.  For example if we had an interface like
   
    ```java
    public interface BlockCacheFactory {
      /**
       * TODO document how this can utilize the new arbitrary instance config
       */
      public BlockCache getBlockCache(AccumuloConfiguration conf);
    }
    ```  
   
    If we had that then we could configure an LruBlockCacheFactory that actually calls the constructor, enabling us to keep using final.
   
    ```java
    public class LruBlockCacheFactory implement BlockCacheFactory {
       public BlockCache getBlockCache(AccumuloConfiguration conf) {
            //read whatever is needed from config in local vars
            return new LruBlockCache(localVar1, localVar2);
       }
    }
    ```
   
    With this, the `start()` method would not be needed also.  The `start()` method makes assumptions about the required config.  Using the configurable factory makes no assumptions about the config needed.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115291623
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -245,7 +245,8 @@
       TSERV_PREFIX("tserver.", null, PropertyType.PREFIX, "Properties in this category affect the behavior of the tablet servers"),
       TSERV_CLIENT_TIMEOUT("tserver.client.timeout", "3s", PropertyType.TIMEDURATION, "Time to wait for clients to continue scans before closing a session."),
       TSERV_DEFAULT_BLOCKSIZE("tserver.default.blocksize", "1M", PropertyType.BYTES, "Specifies a default blocksize for the tserver caches"),
    -  TSERV_CACHE_POLICY("tserver.cache.policy", "LRU", PropertyType.STRING, "Specifies the eviction policy of the file data caches (LRU or TinyLFU)."),
    --- End diff --
   
    I am not sure this was in Accumulo 1.8


---
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
|

[GitHub] accumulo issue #256: ACCUMULO-4463: Make block cache implentation configurab...

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

    https://github.com/apache/accumulo/pull/256
 
    Ok, let me see if I can rework this given the comments up to this point.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115293480
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java ---
    @@ -84,7 +85,7 @@
       static final int statThreadPeriod = 60;
     
       /** Concurrent map (the cache) */
    -  private final ConcurrentHashMap<String,CachedBlock> map;
    --- End diff --
   
    One more thought.. may want to have an enum cache type.
   
    ```java
    enum CacheType {
      INDEX,
      DATA,
      SUMMARY
    }
    ```
   
    Then the factory could key use this.
   
    ```java
    public interface BlockCacheFactory {
      /**
       * TODO document how this can utilize the new arbitrary instance config
       */
      public BlockCache getBlockCache(CacheType ct, AccumuloConfiguration conf);
    }
    ```
   
    Then the implementation could use this for differentiated config.
   
    ```java
    public class SuperDuperBlockCacheFactory implement BlockCacheFactory {
       public BlockCache getBlockCache(CacheType ct, AccumuloConfiguration conf) {
           long knob42 = conf.getMemory("general.custom."+ct.name()+".knob42");
            //read whatever is needed from config in local vars
            return new SuperDuperBlockCacheFactory(knob42);
       }
    }
    ```



---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115293896
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java ---
    @@ -84,7 +85,7 @@
       static final int statThreadPeriod = 60;
     
       /** Concurrent map (the cache) */
    -  private final ConcurrentHashMap<String,CachedBlock> map;
    --- End diff --
   
    The Lru cache factory could use the CacheType to get `"tserver.cache.data.size` or `tserver.cache.index.size`


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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/256#discussion_r115294050
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java ---
    @@ -84,7 +85,7 @@
       static final int statThreadPeriod = 60;
     
       /** Concurrent map (the cache) */
    -  private final ConcurrentHashMap<String,CachedBlock> map;
    --- End diff --
   
    I like these suggestions -- more than my own :)


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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/256#discussion_r115294680
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java ---
    @@ -17,10 +17,30 @@
      */
     package org.apache.accumulo.core.file.blockfile.cache;
     
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
    +
     /**
      * Block cache interface.
      */
     public interface BlockCache {
    +
    +  /**
    +   * Start the block cache
    +   *
    +   * @param conf
    +   *          Accumulo configuration object
    +   * @param maxSize
    +   *          maximum size of the on-heap cache
    +   * @param blockSize
    +   *          size of the default RFile blocks
    +   */
    +  void start(AccumuloConfiguration conf, long maxSize, long blockSize) throws Exception;
    --- End diff --
   
    Hmm, yeah, I later realized most impls would find some way to piggy-back properties into accumulo-site. Mostly, I figured that, eventually, we'd also have other "universal" properties (max size and block size) that all of the implementations would use.
   
    Having some object there (or, using the Factory as Keith's suggestion pushes towards) would at least avoid runtime issues (where the new Accumulo expects a `start(AccumuloConfiguration, long, long, ...)` instead of just `start(AccumuloConfiguration, long, long)`.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115294926
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java ---
    @@ -84,7 +85,7 @@
       static final int statThreadPeriod = 60;
     
       /** Concurrent map (the cache) */
    -  private final ConcurrentHashMap<String,CachedBlock> map;
    --- End diff --
   
    If have a CacheType, then could possibly deprecate the `tserver.cache.data.size` type settings in favor of every implementation using `general.custom.xyz.` settings.  Or we could assume that every implementation will at least want a max size and keep these settings around.  I don't know whats best.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115331908
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -245,7 +245,8 @@
       TSERV_PREFIX("tserver.", null, PropertyType.PREFIX, "Properties in this category affect the behavior of the tablet servers"),
       TSERV_CLIENT_TIMEOUT("tserver.client.timeout", "3s", PropertyType.TIMEDURATION, "Time to wait for clients to continue scans before closing a session."),
       TSERV_DEFAULT_BLOCKSIZE("tserver.default.blocksize", "1M", PropertyType.BYTES, "Specifies a default blocksize for the tserver caches"),
    -  TSERV_CACHE_POLICY("tserver.cache.policy", "LRU", PropertyType.STRING, "Specifies the eviction policy of the file data caches (LRU or TinyLFU)."),
    --- End diff --
   
    It looks like you are correct @keith-turner , this property does not appear in the 1.8 branch.


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115348299
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -245,7 +245,8 @@
       TSERV_PREFIX("tserver.", null, PropertyType.PREFIX, "Properties in this category affect the behavior of the tablet servers"),
       TSERV_CLIENT_TIMEOUT("tserver.client.timeout", "3s", PropertyType.TIMEDURATION, "Time to wait for clients to continue scans before closing a session."),
       TSERV_DEFAULT_BLOCKSIZE("tserver.default.blocksize", "1M", PropertyType.BYTES, "Specifies a default blocksize for the tserver caches"),
    -  TSERV_CACHE_POLICY("tserver.cache.policy", "LRU", PropertyType.STRING, "Specifies the eviction policy of the file data caches (LRU or TinyLFU)."),
    +  TSERV_CACHE_IMPL("tserver.cache.factory.class", "org.apache.accumulo.core.file.blockfile.cache.lru.LRUBlockCacheFactory.class", PropertyType.STRING,
    --- End diff --
   
    Should include `FACTORY` in the enum name.  Maybe call it `TSERV_CACHE_FACTORY` or `TSERV_CACHE_FACTORY_IMPL`


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115347395
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCacheFactory.java ---
    @@ -0,0 +1,33 @@
    +/*
    + * 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.file.blockfile.cache;
    +
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader;
    +
    +public abstract class BlockCacheFactory {
    +
    +  public static BlockCacheFactory getBlockCacheFactory(AccumuloConfiguration conf) throws Exception {
    +    String impl = conf.get(Property.TSERV_CACHE_IMPL);
    +    Class<? extends BlockCacheFactory> clazz = AccumuloVFSClassLoader.loadClass(impl, BlockCacheFactory.class);
    +    return clazz.newInstance();
    +  }
    +
    +  public abstract BlockCache getBlockCache(AccumuloConfiguration conf);
    --- End diff --
   
    This method could have java with `@see BlockCacheConfiguration a helper class for configuring cache instances`


---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115350017
 
    --- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java ---
    @@ -175,19 +176,27 @@ public TabletServerResourceManager(TabletServer tserver, VolumeManager fs) {
         long sCacheSize = acuConf.getAsBytes(Property.TSERV_SUMMARYCACHE_SIZE);
         long totalQueueSize = acuConf.getAsBytes(Property.TSERV_TOTAL_MUTATION_QUEUE_MAX);
     
    -    String policy = acuConf.get(Property.TSERV_CACHE_POLICY);
    -    if (policy.equalsIgnoreCase("LRU")) {
    -      _iCache = new LruBlockCache(iCacheSize, blockSize);
    -      _dCache = new LruBlockCache(dCacheSize, blockSize);
    -      _sCache = new LruBlockCache(sCacheSize, blockSize);
    -    } else if (policy.equalsIgnoreCase("TinyLFU")) {
    -      _iCache = new TinyLfuBlockCache(iCacheSize, blockSize);
    -      _dCache = new TinyLfuBlockCache(dCacheSize, blockSize);
    -      _sCache = new TinyLfuBlockCache(sCacheSize, blockSize);
    -    } else {
    -      throw new IllegalArgumentException("Unknown Block cache policy " + policy);
    +    BlockCacheFactory factory;
    +    try {
    +      factory = BlockCacheFactory.getBlockCacheFactory(acuConf);
    +    } catch (Exception e) {
    +      throw new RuntimeException("Error creating Block Cache Factory", e);
         }
     
    +    ConfigurationCopy copy = new ConfigurationCopy(acuConf);
    +    copy.set(BlockCacheConfiguration.BLOCK_SIZE_PROPERTY, Long.toString(blockSize));
    +    copy.set(BlockCacheConfiguration.MAX_SIZE_PROPERTY, Long.toString(iCacheSize));
    --- End diff --
   
    This behavior is kind of tricky.  What if these properties were already set in acuConf, what should the behavior be?  The javadoc for BlockCacheFactory would need to mention this behavior.  
   



---
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
|

[GitHub] accumulo pull request #256: ACCUMULO-4463: Make block cache implentation con...

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

    https://github.com/apache/accumulo/pull/256#discussion_r115347713
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/lru/LruBlockCacheConfiguration.java ---
    @@ -0,0 +1,122 @@
    +/*
    + * 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.file.blockfile.cache.lru;
    +
    +import java.util.Map;
    +
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.core.file.blockfile.cache.BlockCacheConfiguration;
    +
    +public final class LruBlockCacheConfiguration extends BlockCacheConfiguration {
    --- End diff --
   
    Nice that this class is final and all of its members a final.  I am not sure if LruBlockCache accesses these frequently, but if it does making stuff final will help.


---
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.
---
12345