----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66085/#review199382 -----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 37 (patched) <https://reviews.apache.org/r/66085/#comment279706> Consider using already existing EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_ATLAS_NAME, instead of defining new constant SERVICEDBSTORE_SERVICEDEFBYNAME_ATLAS_NAME. security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 90 (patched) <https://reviews.apache.org/r/66085/#comment279701> Why is it critical to bail out when "serviceId != 11"? If the intention is to avoid rename on multiple runs of this patch, then I would suggest to check for resource-names and access-types to determine if the service-def is older version or not. If the service-def has all resources and accessTypes listed below, then it is an older service-def and should be renamed: - resources: entity, type, operation, taxonomy, term - accessTypes: read, create, update, delete, all security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 91 (patched) <https://reviews.apache.org/r/66085/#comment279702> add a info level log here. LOG.info(EMBEDDED_SERVICEDEF_ATLAS_NAME + ": service-def not found. No patching is needed"); security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 95 (patched) <https://reviews.apache.org/r/66085/#comment279707> Consider renaming service-def first. The suffix used to rename the service-def should be used to rename the services as well. String serviceDefName = EMBEDDED_SERVICEDEF_ATLAS_NAME; XXServiceDefDao serviceDefDao = daoMgr.getXXServiceDef(); XXServiceDef serviceDef = serviceDefDao.findByName(serviceDefName); if (serviceDef == null) { LOG.info(serviceDefName + ": service-def not found. No patching is needed"); return; } String suffix = null; for (int i = 1; true; i++) { suffix = ".v" + i; if (serviceDefDao.findByName(serviceDefName + suffix) == null) { break; } } String serviceDefNewName = serviceDefName + suffix; LOG.info("Renaming service-def " + serviceDefName + " as " + serviceDefNewName); serviceDef.setName(serviceDefNewName); serviceDefDao.update(serviceDef); LOG.info("Renamed service-def " + serviceDefName + " as " + serviceDefNewName); XXServiceDao serviceDao = daoMgr.getXXService(); List<XXService> services = serviceDao.findByServiceDefId(serviceDef.getId()); if (CollectionUtils.isNotEmpty(services)) { for (XXService service : services) { String serviceName = service.getName(); String serviceNewName = serviceName + suffix; LOG.info("Renaming service " + serviceName + " as " + serviceNewName); if (serviceDao.findByName(serviceNewName) != null) { LOG.warn("Another service named " + serviceNewName + " already exists. Not renaming " + serviceName); continue; } service.setName(serviceNewName); serviceDao.update(service); LOG.info("Renamed service " + serviceName + " as " + serviceNewName); } } security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 103 (patched) <https://reviews.apache.org/r/66085/#comment279703> When would this be false? Should that be considered as a patch failure (instead of ignoring it)? Atleast a warning should be logged. security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 104 (patched) <https://reviews.apache.org/r/66085/#comment279704> "Renamed service " + service.getName() + " to " updateServiceName security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java Lines 111 (patched) <https://reviews.apache.org/r/66085/#comment279705> When would this be false? I think failing to rename should be considered as a patch failure. - Madhan Neethiraj On March 17, 2018, 9:33 p.m., Pradeep Agrawal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66085/ > ----------------------------------------------------------- > > (Updated March 17, 2018, 9:33 p.m.) > > > Review request for ranger, bhavik patel, Gautam Borad, Abhay Kulkarni, Madhan > Neethiraj, Mehul Parikh, Ramesh Mani, Sailaja Polavarapu, and Velmurugan > Periasamy. > > > Bugs: RANGER-2019 > https://issues.apache.org/jira/browse/RANGER-2019 > > > Repository: ranger > > > Description > ------- > > **Problem Statement:** Based on this RANGER-2004 commit, Atlas service def is > updated. During the upgrade of existing env, old Atlas service def should be > renamed so that new service def can be used. > > **Proposed Solution:** Proposed Java patch can address the atlas service def > rename fix. During migration to latest Ranger, this Java patch should do the > following: > 1. Check for service-def named 'atlas'. If it is not found - nothing more to > do. > 2. Rename all service instances of type 'atlas', with addition of suffix > "-v1". Existing service named 'cl1_atlas' will become 'cl1_atlas-v1'. If > 'cl1_atlas-v1' already exists, append an count like 'cl1_atlas-v1.1', > 'cl1_atlas-v1.2' > 3. After all service instances are renamed, rename service-def as 'atlas-v1'. > > **Note:** In Upgrade case, as the old atlas service def id can't be used to > create the new atlas service def so I am proposing change to update the atlas > servicedef id from 11 to 15. > > > Diffs > ----- > > agents-common/src/main/resources/service-defs/ranger-servicedef-atlas.json > 2891129 > > security-admin/src/main/java/org/apache/ranger/patch/PatchForAtlasServiceDefUpdate_J10013.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66085/diff/1/ > > > Testing > ------- > > Tested for fresh and upgrade case. > > > Thanks, > > Pradeep Agrawal > >
