[jira] [Commented] (SENTRY-1683) MetastoreCacheInitializer has a race condition in handling results list

["Hadoop QA (JIRA)" <ji...@apache.org>]

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

Hadoop QA commented on SENTRY-1683:
-----------------------------------

Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12861532/SENTRY-1683.001.patch against master.

{color:green}Overall:{color} +1 all checks pass

{color:green}SUCCESS:{color} all tests passed

Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/2467/console

This message is automatically generated.

> MetastoreCacheInitializer has a race condition in handling results list
> -----------------------------------------------------------------------
>
>                 Key: SENTRY-1683
>                 URL: https://issues.apache.org/jira/browse/SENTRY-1683
>             Project: Sentry
>          Issue Type: Bug
>          Components: Sentry
>    Affects Versions: 1.8.0
>            Reporter: Alexander Kolbasov
>            Assignee: Alexander Kolbasov
>         Attachments: SENTRY-1683.001.patch, SENTRY-1683.001.patch
>
>
> The {{MetastoreCacheInitializer}} has the following logic:
> 1) It creates a list of futures to hold results of executors ({{results}})
> 2) {{CreateInitialUpdate() }}does this:
> {code}
>   UpdateableAuthzPaths createInitialUpdate() throws
>           Exception {
>     PathsUpdate tempUpdate = new PathsUpdate(-1, false);
>     List<String> allDbStr = hmsHandler.get_all_databases();
>     for (String dbName : allDbStr) {
>       Callable<CallResult> dbTask = new DbTask(tempUpdate, dbName);
>       results.add(threadPool.submit(dbTask));
>     }
>     while (taskCounter.get() > 0) {
>       Thread.sleep(1000);
>       // Wait until no more tasks remain
>     }
>     for (Future<CallResult> result : results) {
>       CallResult callResult = result.get();
>       // Fail the HMS startup if tasks are not all successful and
>       // fail on partial updates flag is set in the config.
>       if (!callResult.getSuccessStatus() && failOnRetry) {
>         throw callResult.getFailure();
>       }
>     }
>     authzPaths.updatePartial(Lists.newArrayList(tempUpdate),
>             new ReentrantReadWriteLock());
>     return authzPaths;
>   }
> {code}
> Note that in both cases the results list is accessed without holding any locks. 
> But the list is also updated from tasks themselves:
> {code}
>   class DbTask extends BaseTask {
>     @Override
>     public void doTask() throws TException, SentryMalformedPathException {
>      ...
>       List<String> allTblStr = hmsHandler.get_all_tables(dbName);
>       for (int i = 0; i < allTblStr.size(); i += maxTablesPerCall) {
>         synchronized (results) {
>           results.add(threadPool.submit(tableTask));
>         }
>       }
> {code}
> There is some synchronization which uses taskCounter so the second walk of the list happens when all tasks are complete, but the first walk isn't thread-safe  - the list may be updated while initial tasks are created there. Note that some access are synchronized and some are not.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)