[ 
https://issues.apache.org/jira/browse/IMPALA-10130?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Abhishek Rawat updated IMPALA-10130:
------------------------------------
    Description: 
When the catalog service is restarted, LocalCatalog detects it in the topic 
update due to the change in its service id and invalidates its cache contents. 
However, it looks like it doesn't invalidate its existing authPolicy cache 
contents.
{code:java}
  private void witnessCatalogServiceId(TUniqueId serviceId) {
    synchronized (catalogServiceIdLock_) {
      if (!catalogServiceId_.equals(serviceId)) {
        if (!catalogServiceId_.equals(Catalog.INITIAL_CATALOG_SERVICE_ID)) {
          LOG.warn("Detected catalog service restart: service ID changed from " 
+
              "{} to {}. Invalidating all cached metadata on this coordinator.",
              catalogServiceId_, serviceId);
        }
        catalogServiceId_ = serviceId;
        cache_.invalidateAll();
        // Clear cached items from the previous catalogd instance. Otherwise, 
we'll
        // ignore new updates from the new catalogd instance since they have 
lower
        // versions.
        hdfsCachePools_.clear();
        // TODO(todd): we probably need to invalidate the auth policy too.
        // we are probably better off detecting this at a higher level and
        // reinstantiating the metaprovider entirely, similar to how 
ImpaladCatalog
        // handles this.

        // TODO(todd): slight race here: a concurrent request from the old 
catalog
        // could theoretically be just about to write something back into the 
cache
        // after we do the above invalidate. Maybe we would be better off 
replacing
        // the whole cache object, or doing a soft barrier here to wait for any
        // concurrent cache accessors to cycle out. Another option is to 
associate
        // the catalog service ID as part of all of the cache keys.
        //
        // This is quite unlikely to be an issue in practice, so deferring it 
to later
        // clean-up.
      }
    }
  }
{code}
If the older authpolicy is not cleared above, it is possible that when the 
principle was added into the cache it was ignored since its catalog version was 
higher as seen below:
{code:java}
  public synchronized void addPrincipal(Principal principal) {
    Principal existingPrincipal = getPrincipal(principal.getName(),
        principal.getPrincipalType());
    // There is already a newer version of this principal in the catalog, ignore
    // just return.
    if (existingPrincipal != null &&
        existingPrincipal.getCatalogVersion() >= principal.getCatalogVersion()) 
return;
{code}
When the update tries to add the privilege associated with the principle above, 
it looks up the principal using id instead of name and if there is a id 
mismatch it throws error.
{code:java}
  /**
   * Adds a new privilege to the policy mapping to the principal specified by 
the
   * principal ID in the privilege. Throws a CatalogException no principal with 
a
   * corresponding ID existing in the catalog.
   */
  public synchronized void addPrivilege(PrincipalPrivilege privilege)
      throws CatalogException {
    if (LOG.isTraceEnabled()) {
      LOG.trace("Adding privilege: " + privilege.getName() + " " +
          Principal.toString(privilege.getPrincipalType()).toLowerCase() +
          " ID: " + privilege.getPrincipalId());
    }
    Principal principal = getPrincipal(privilege.getPrincipalId(),
        privilege.getPrincipalType());
    if (principal == null) {
      throw new CatalogException(String.format("Error adding privilege: %s. %s 
ID " +
          "'%d' does not exist.", privilege.getName(),
          Principal.toString(privilege.getPrincipalType()), 
privilege.getPrincipalId()));
    }{code}
 

The legacy catalog mode doesn't have this issue because the whole 
ImpaladCatalog instance is re-created when detecting catalogd restarts:
{code:java}
    @Override
    TUpdateCatalogCacheResponse updateCatalogCache(TUpdateCatalogCacheRequest 
req)
        throws CatalogException, TException {
      ImpaladCatalog catalog = catalog_.get();
      if (req.is_delta) return catalog.updateCatalog(req);

      // If this is not a delta, this update should replace the current
      // Catalog contents so create a new catalog and populate it.
      catalog = createNewCatalog();

      TUpdateCatalogCacheResponse response = catalog.updateCatalog(req);

      // Now that the catalog has been updated, replace the reference to
      // catalog_. This ensures that clients don't see the catalog
      // disappear. The catalog is guaranteed to be ready since updateCatalog() 
has a
      // postcondition of isReady() == true.
      catalog_.set(catalog);
      return response;
    }
{code}

  was:
When the catalog service is restarted, LocalCatalog detects it in the topic 
update due to the change in its service id and invalidates its cache contents. 
However, it looks like it doesn't invalidate its existing authPolicy cache 
contents.
{code:java}
  private void witnessCatalogServiceId(TUniqueId serviceId) {
    synchronized (catalogServiceIdLock_) {
      if (!catalogServiceId_.equals(serviceId)) {
        if (!catalogServiceId_.equals(Catalog.INITIAL_CATALOG_SERVICE_ID)) {
          LOG.warn("Detected catalog service restart: service ID changed from " 
+
              "{} to {}. Invalidating all cached metadata on this coordinator.",
              catalogServiceId_, serviceId);
        }
        catalogServiceId_ = serviceId;
        cache_.invalidateAll();
        // Clear cached items from the previous catalogd instance. Otherwise, 
we'll
        // ignore new updates from the new catalogd instance since they have 
lower
        // versions.
        hdfsCachePools_.clear();
        // TODO(todd): we probably need to invalidate the auth policy too.
        // we are probably better off detecting this at a higher level and
        // reinstantiating the metaprovider entirely, similar to how 
ImpaladCatalog
        // handles this.

        // TODO(todd): slight race here: a concurrent request from the old 
catalog
        // could theoretically be just about to write something back into the 
cache
        // after we do the above invalidate. Maybe we would be better off 
replacing
        // the whole cache object, or doing a soft barrier here to wait for any
        // concurrent cache accessors to cycle out. Another option is to 
associate
        // the catalog service ID as part of all of the cache keys.
        //
        // This is quite unlikely to be an issue in practice, so deferring it 
to later
        // clean-up.
      }
    }
  }
{code}
If the older authpolicy is not cleared above, it is possible that when the 
principle was added into the cache it was ignored since its catalog version was 
higher as seen below:
{code:java}
  public synchronized void addPrincipal(Principal principal) {
    Principal existingPrincipal = getPrincipal(principal.getName(),
        principal.getPrincipalType());
    // There is already a newer version of this principal in the catalog, ignore
    // just return.
    if (existingPrincipal != null &&
        existingPrincipal.getCatalogVersion() >= principal.getCatalogVersion()) 
return;
{code}
When the update tries to add the privilege associated with the principle above, 
it looks up the principal using id instead of name and if there is a id 
mismatch it throws error.
{code:java}
  /**
   * Adds a new privilege to the policy mapping to the principal specified by 
the
   * principal ID in the privilege. Throws a CatalogException no principal with 
a
   * corresponding ID existing in the catalog.
   */
  public synchronized void addPrivilege(PrincipalPrivilege privilege)
      throws CatalogException {
    if (LOG.isTraceEnabled()) {
      LOG.trace("Adding privilege: " + privilege.getName() + " " +
          Principal.toString(privilege.getPrincipalType()).toLowerCase() +
          " ID: " + privilege.getPrincipalId());
    }
    Principal principal = getPrincipal(privilege.getPrincipalId(),
        privilege.getPrincipalType());
    if (principal == null) {
      throw new CatalogException(String.format("Error adding privilege: %s. %s 
ID " +
          "'%d' does not exist.", privilege.getName(),
          Principal.toString(privilege.getPrincipalType()), 
privilege.getPrincipalId()));
    }{code}
 

The legacy catalog mode doesn't have this issue because the whole 
ImpaladCatalog instance is re-created when detecting catalogd restarts:
    @Override
    TUpdateCatalogCacheResponse updateCatalogCache(TUpdateCatalogCacheRequest 
req)        throws CatalogException, TException {
      ImpaladCatalog catalog = catalog_.get();      if (req.is_delta) return 
catalog.updateCatalog(req);      // If this is not a delta, this update should 
replace the current      // Catalog contents so create a new catalog and 
populate it.      catalog = createNewCatalog();

      TUpdateCatalogCacheResponse response = catalog.updateCatalog(req);      
// Now that the catalog has been updated, replace the reference to      // 
catalog_. This ensures that clients don't see the catalog      // disappear. 
The catalog is guaranteed to be ready since updateCatalog() has a      // 
postcondition of isReady() == true.      catalog_.set(catalog);      return 
response;
    }


> Catalog restart doesn't invalidate authPolicy cache in local catalog
> --------------------------------------------------------------------
>
>                 Key: IMPALA-10130
>                 URL: https://issues.apache.org/jira/browse/IMPALA-10130
>             Project: IMPALA
>          Issue Type: Bug
>          Components: Catalog
>    Affects Versions: Impala 2.0, Impala 3.0, Impala 4.0
>            Reporter: Abhishek Rawat
>            Priority: Major
>
> When the catalog service is restarted, LocalCatalog detects it in the topic 
> update due to the change in its service id and invalidates its cache 
> contents. However, it looks like it doesn't invalidate its existing 
> authPolicy cache contents.
> {code:java}
>   private void witnessCatalogServiceId(TUniqueId serviceId) {
>     synchronized (catalogServiceIdLock_) {
>       if (!catalogServiceId_.equals(serviceId)) {
>         if (!catalogServiceId_.equals(Catalog.INITIAL_CATALOG_SERVICE_ID)) {
>           LOG.warn("Detected catalog service restart: service ID changed from 
> " +
>               "{} to {}. Invalidating all cached metadata on this 
> coordinator.",
>               catalogServiceId_, serviceId);
>         }
>         catalogServiceId_ = serviceId;
>         cache_.invalidateAll();
>         // Clear cached items from the previous catalogd instance. Otherwise, 
> we'll
>         // ignore new updates from the new catalogd instance since they have 
> lower
>         // versions.
>         hdfsCachePools_.clear();
>         // TODO(todd): we probably need to invalidate the auth policy too.
>         // we are probably better off detecting this at a higher level and
>         // reinstantiating the metaprovider entirely, similar to how 
> ImpaladCatalog
>         // handles this.
>         // TODO(todd): slight race here: a concurrent request from the old 
> catalog
>         // could theoretically be just about to write something back into the 
> cache
>         // after we do the above invalidate. Maybe we would be better off 
> replacing
>         // the whole cache object, or doing a soft barrier here to wait for 
> any
>         // concurrent cache accessors to cycle out. Another option is to 
> associate
>         // the catalog service ID as part of all of the cache keys.
>         //
>         // This is quite unlikely to be an issue in practice, so deferring it 
> to later
>         // clean-up.
>       }
>     }
>   }
> {code}
> If the older authpolicy is not cleared above, it is possible that when the 
> principle was added into the cache it was ignored since its catalog version 
> was higher as seen below:
> {code:java}
>   public synchronized void addPrincipal(Principal principal) {
>     Principal existingPrincipal = getPrincipal(principal.getName(),
>         principal.getPrincipalType());
>     // There is already a newer version of this principal in the catalog, 
> ignore
>     // just return.
>     if (existingPrincipal != null &&
>         existingPrincipal.getCatalogVersion() >= 
> principal.getCatalogVersion()) return;
> {code}
> When the update tries to add the privilege associated with the principle 
> above, it looks up the principal using id instead of name and if there is a 
> id mismatch it throws error.
> {code:java}
>   /**
>    * Adds a new privilege to the policy mapping to the principal specified by 
> the
>    * principal ID in the privilege. Throws a CatalogException no principal 
> with a
>    * corresponding ID existing in the catalog.
>    */
>   public synchronized void addPrivilege(PrincipalPrivilege privilege)
>       throws CatalogException {
>     if (LOG.isTraceEnabled()) {
>       LOG.trace("Adding privilege: " + privilege.getName() + " " +
>           Principal.toString(privilege.getPrincipalType()).toLowerCase() +
>           " ID: " + privilege.getPrincipalId());
>     }
>     Principal principal = getPrincipal(privilege.getPrincipalId(),
>         privilege.getPrincipalType());
>     if (principal == null) {
>       throw new CatalogException(String.format("Error adding privilege: %s. 
> %s ID " +
>           "'%d' does not exist.", privilege.getName(),
>           Principal.toString(privilege.getPrincipalType()), 
> privilege.getPrincipalId()));
>     }{code}
>  
> The legacy catalog mode doesn't have this issue because the whole 
> ImpaladCatalog instance is re-created when detecting catalogd restarts:
> {code:java}
>     @Override
>     TUpdateCatalogCacheResponse updateCatalogCache(TUpdateCatalogCacheRequest 
> req)
>         throws CatalogException, TException {
>       ImpaladCatalog catalog = catalog_.get();
>       if (req.is_delta) return catalog.updateCatalog(req);
>       // If this is not a delta, this update should replace the current
>       // Catalog contents so create a new catalog and populate it.
>       catalog = createNewCatalog();
>       TUpdateCatalogCacheResponse response = catalog.updateCatalog(req);
>       // Now that the catalog has been updated, replace the reference to
>       // catalog_. This ensures that clients don't see the catalog
>       // disappear. The catalog is guaranteed to be ready since 
> updateCatalog() has a
>       // postcondition of isReady() == true.
>       catalog_.set(catalog);
>       return response;
>     }
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to