hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Bota (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HADOOP-13649) s3guard: implement time-based (TTL) expiry for LocalMetadataStore
Date Wed, 25 Apr 2018 15:18:00 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-13649?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16452447#comment-16452447
] 

Gabor Bota edited comment on HADOOP-13649 at 4/25/18 3:17 PM:
--------------------------------------------------------------

Comments on my 001 patch:
1. Removed the LruHasMap implementation
 - Using the com.google.common.cache.Cache
 - All tests pass
 - Loosing the mruGet method. I was not able to find any test for this particular feature,
or 'real world' usage besides that it will modify the LinkedList inside the LinkedHashMap,
so it will have the most recently modified elements in the start of the List. I don't think
that would cause big performance issue in testing, but I've made a little benchmark to test
this.[1]
 - Using basically the same implementation in LocalMetadataStore, the new addition is only
the Timed Eviction.
 - Of course I had to rename every hash to cache, and there were some other minor changes.

2. Created a test for the Timed Eviction (testCacheTimedEvictionAfterWrite)
 - Only in the TestLocalMetadataStore (not in the abstract test)
 - Just tests if the cache implementation inside LocalMetadataStore working as expected, nothing
more.
 - Maybe additional tests could be added for higher level functionality, but the problem is
that there will be a need from the test to change the default cache inside the instance to
another cache build with the support for a custom Ticker to avoid realtime waiting for the
eviction (which could cause flakyness).

3. Should the removal of the evicted elements be logged?
 - There is an option to log removed elements. It can be done synchronously (default), and
asynchronously as well. Using the synchronous way could be expensive, and I'm not even sure
if we want this (maybe good for debug) so I haven't included it in the initial patch.

4. Maybe DEFAULT_EXPIRY_AFTER_WRITE_SECONDS should be DEFAULT_EVICTION_AFTER_WRITE_SECONDS

[1] The impact of having cache instead of LruHashMap in LocalMetadataStore on tests:
 - mvn -Dparallel-tests -DtestsThreadCount=8 clean verify
with patch: 4m10.917s
without patch: 4m12.413s
 - mvn -Dparallel-tests -DtestsThreadCount=8 clean test
with patch: 4m6.383s
without patch: 4m5.217s

Test and verify runs on us-west-2 succesfully for the patch.


was (Author: gabor.bota):
Comments on my 001 patch:
1. Removed the LruHasMap implementation
 - Using the com.google.common.cache.Cache
 - All tests pass
 - Loosing the mruGet method. I was not able to find any test for this particular feature,
or 'real world' usage besides that it will modify the LinkedList inside
the LinkedHashMap, so it will have the most recently modified elements
in the start of the List. I don't think that would cause big performance
issue in testing, but I've made a little benchmark to test this.[1]
 - Using basically the same implementation in LocalMetadataStore, the
new addition is only the Timed Eviction.
 - Of course I had to rename every hash to cache, and there were some other minor changes.

2. Created a test for the Timed Eviction (testCacheTimedEvictionAfterWrite)
 - Only in the TestLocalMetadataStore (not in the abstract test)
 - Just tests if the cache implementation inside LocalMetadataStore working
as expected, nothing more.
 - Maybe additional tests could be added for higher level functionality, but the
problem is that there will be a need from the test to change the default
cache inside the instance to another cache build with the support for a custom Ticker
to avoid realtime waiting for the eviction (which could cause flakyness).

3. Should the removal of the evicted elements be logged?
 - There is an option to log removed elements. It can be done synchronously (default),
and asynchronously as well. Using the synchronous way could be expensive, and I'm
not even sure if we want this (maybe good for debug) so I haven't included it in the
initial patch.

4. Maybe DEFAULT_EXPIRY_AFTER_WRITE_SECONDS should be DEFAULT_EVICTION_AFTER_WRITE_SECONDS

[1] The impact of having cache instead of LruHashMap in LocalMetadataStore on tests:
 - mvn -Dparallel-tests -DtestsThreadCount=8 clean verify
with patch: 4m10.917s
without patch: 4m12.413s
 - mvn -Dparallel-tests -DtestsThreadCount=8 clean test
with patch: 4m6.383s
without patch: 4m5.217s

Test and verify runs on us-west-2 succesfully for the patch.

> s3guard: implement time-based (TTL) expiry for LocalMetadataStore
> -----------------------------------------------------------------
>
>                 Key: HADOOP-13649
>                 URL: https://issues.apache.org/jira/browse/HADOOP-13649
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: fs/s3
>    Affects Versions: 3.0.0-beta1
>            Reporter: Aaron Fabbri
>            Assignee: Gabor Bota
>            Priority: Minor
>         Attachments: HADOOP-13649.001.patch
>
>
> LocalMetadataStore is primarily a reference implementation for testing.  It may be useful
in narrow circumstances where the workload can tolerate short-term lack of inter-node consistency:
 Being in-memory, one JVM/node's LocalMetadataStore will not see another node's changes to
the underlying filesystem.
> To put a bound on the time during which this inconsistency may occur, we should implement
time-based (a.k.a. Time To Live / TTL)  expiration for LocalMetadataStore



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message