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

Alexander Kolbasov commented on SENTRY-1491:
--------------------------------------------

I think we should reorganize SentryStore code to have have a common handling 
for transactions. Here is what I suggest.

1) Use try-with-resources block for closing PersistenceManager. This requires a 
version of PersistenceManager that implements AutoCloseable (see SENTRY=1500). 
So we can have something like

{code}
  /**
   * CloseablePersistenceManager is a wrapper around PersistenceManager that
   * implements AutoCloseable interface. It is needed because Apache jdo doesn't
   * implement AutoCloseable (Datanucleus version does).
   */
  class CloseablePersistenceManager implements AutoCloseable {
    public final PersistenceManager pm;

    CloseablePersistenceManager() {
      pm = pmf.getPersistenceManager();
    }

    @Override
    public void close() throws Exception {
      pm.close();
    }

    public Transaction getTransaction() {
      return pm.currentTransaction();
    }
  }
{code}

2) We want to have a common code that handles transactions/rollbacks. Instead 
of using the current scheme I suggest using the following one:

{code}
  /**
   * TransactionBlock is a piece of code that is executed inside a single
   * transaction
   */
  interface TransactionBlock {
    public Object execute(PersistenceManager pm) throws Exception;
  }

  /**
   * Execute some code as a single transaction
   * @param tb transaction block with code to execute
   * @return Object with the result of tb.execute()
   * @throws Exception
     */
  public Object ExecuteTransaction(TransactionBlock tb) throws Exception
  {
    try(CloseablePersistenceManager cpm = new CloseablePersistenceManager()) {
      Transaction tx = cpm.getTransaction();
      tx.begin();
      try {
        Object result = tb.execute(cpm.pm);
        tx.commit();
        return result;
      } finally {
        if (tx.isActive()) {
          tx.rollback();
        }
      }
    }
  }
{code}

And then use something like this:

{code}
  /**
   * Create a sentry role and persist it.
   * @param roleName: Name of the role being persisted
   * @returns commit context used for notification handlers
   * @throws SentryAlreadyExistsException
   */
  public CommitContext createSentryRole(String roleName)
          throws Exception {

    class CreateSentryTransaction implements TransactionBlock {
      final String roleName;

      CreateSentryTransaction(String roleName) {
        this.roleName = roleName;
      }

      @Override
      public Object execute(PersistenceManager pm) throws Exception {
        MSentryRole mSentryRole = getMSentryRole(pm, roleName);
        if (mSentryRole == null) {
          MSentryRole mRole = new MSentryRole(roleName,
                  System.currentTimeMillis());
          pm.makePersistent(mRole);
          return new CommitContext();
        } else {
          throw new SentryAlreadyExistsException("Role: " + roleName);
        }
      }
    }

    return (CommitContext)ExecuteTransaction(
            new CreateSentryTransaction(trimAndLower(roleName)));
  }
{code}

> Sentry transactions are not rolled back immediately when commit fails
> ---------------------------------------------------------------------
>
>                 Key: SENTRY-1491
>                 URL: https://issues.apache.org/jira/browse/SENTRY-1491
>             Project: Sentry
>          Issue Type: Bug
>          Components: Sentry
>    Affects Versions: 1.7.0, sentry-ha-redesign
>            Reporter: Alexander Kolbasov
>            Assignee: Alexander Kolbasov
>
> Here is the code for commitTransaction:
> {code}
> public void commitTransaction(PersistenceManager pm) {
>   Transaction currentTransaction = pm.currentTransaction();
>   try {
>     Preconditions.checkState(currentTransaction.isActive(), "Transaction is 
> not active");
>     currentTransaction.commit();
>   } finally {
>     pm.close();
>   }
> }
> {code}
> so it does pm.close() in the finally clause, so it always closes pm. 
> Now everyone else does something like
> {code}
>   commitTransaction(pm);
>   rollbackTransaction = false;
>   return roles;
> } finally {
>   if (rollbackTransaction) {
>     rollbackTransaction(pm);
>   }
> }
> {code}
> which means that pm is closed by the time we call rollbackTransaction() and 
> we never rollback when commit fails. As a result rollback is deferred till 
> connection close (which may be some time due to connection pooling) and we 
> are potentially holding various locks for longer than needed.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to