mneethiraj commented on code in PR #545:
URL: https://github.com/apache/ranger/pull/545#discussion_r2004621294


##########
security-admin/src/main/resources/META-INF/jpa_named_queries.xml:
##########
@@ -120,7 +120,10 @@
                 order by obj.policyId, obj.id
                         </query>
         </named-query>
-
+               <named-query name="XXPolicyLabelMap.findByPolicyIdAndLabelId">
+                       <query>SELECT obj FROM XXPolicyLabelMap obj WHERE 
obj.id = :policyLabelId AND obj.policyId = :policyId

Review Comment:
   `obj.id = :policyLabelId` ==> `obj.policyLabelId = :policyLabelId`



##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -6741,15 +6741,28 @@ public void run() {
             getOrCreateLabel();
         }
 
+        private boolean doesPolicyExist(XXPolicy xPolicy) {
+            XXPolicy     xxExisting = 
daoMgr.getXXPolicy().getById(xPolicy.getId());
+            LOG.info("Found existing policy {}", xxExisting);
+            RangerPolicy existing   = 
policyService.getPopulatedViewObject(xxExisting);

Review Comment:
   Is it necessary to get `RangerPolicy` from `xxExisting`? Why not `return 
xxExisting != null;` 



##########
security-admin/src/main/java/org/apache/ranger/common/db/RangerTransactionSynchronizationAdapter.java:
##########
@@ -206,8 +205,18 @@ private void runRunnables(final List<Runnable> runnables, 
final boolean isParent
                             LOG.debug("Failed to commit TransactionService 
transaction, throwable:[{}]", String.valueOf(e));
                         }
                     }
-                }
-                while (isParentTransactionCommitted && 
!isThisTransactionCommitted);
+
+                    isThisTransactionCommitted = result == runnable;
+
+                    if (isParentTransactionCommitted) {
+                        if (!isThisTransactionCommitted) {
+                            LOG.info("Failed to commit runnable:[{}]. Will 
retry!", runnable);
+                            count ++;
+                        } else {
+                            LOG.debug("Committed runnable:[{}].", runnable);
+                        }
+                    }
+                } while (isParentTransactionCommitted && 
!isThisTransactionCommitted && count < 5);

Review Comment:
   Why is it necessary to limit the number of retries? The right fix would be 
to update Runnable implementations to return success when the objects 
referenced (policy, label, ..) don't exist in the database any more.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@ranger.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to