You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by "fateh288 (via GitHub)" <gi...@apache.org> on 2025/05/01 17:53:20 UTC
[PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
fateh288 opened a new pull request, #569:
URL: https://github.com/apache/ranger/pull/569
…er/KMS
## What changes were proposed in this pull request?
A new framework using liquidate to support ZDU
## How was this patch tested?
Unit tests have been added
Tested in a private cluster
WIP: Testing in docker setup
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2114805056
##########
kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStoreProvider.java:
##########
@@ -66,7 +66,7 @@ public class RangerKeyStoreProvider extends KeyProvider {
private static final String CREDENTIAL_PATH = "ranger.ks.jpa.jdbc.credential.provider.path";
private static final String MK_CREDENTIAL_ALIAS = "ranger.ks.masterkey.credential.alias";
private static final String DB_CREDENTIAL_ALIAS = "ranger.ks.jpa.jdbc.credential.alias";
- private static final String DB_PASSWORD = "ranger.ks.jpa.jdbc.password";
+ public static final String DB_PASSWORD = "ranger.ks.jpa.jdbc.password";
Review Comment:
Changing DB_PASSWORD from private to public can affect the module's API. Please ensure this change is intentional and that the reasoning is clearly documented.
##########
liquibase-database-upgrade/changelogs/kms/db/test_3.x/kms_rename_col_zdu.xml:
##########
@@ -0,0 +1,56 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<databaseChangeLog
+ xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+ xmlns:pro="http://www.liquibase.org/xml/ns/pro"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+ http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd
+ http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd
+ http://www.liquibase.org/xml/ns/pro http://www.liquibase.org/xml/ns/pro/liquibase-pro-latest.xsd">
+ <changeSet id="5a" author="fateh">
+ <addColumn tableName="ranger_keystore">
+ <column name="kms_encoded_value"
+ type="varchar(2048)"/>
+ </addColumn>
+ <!-- <rollback>-->
+ <!-- <!– Drop constraints/indexes that reference the new column (if any)-->
+ <!--<sql>ALTER TABLE ranger_keystore DROP CONSTRAINT constraint_name;</sql> –>-->
+ <!-- <!– Drop the old column –>-->
+ <!-- <dropColumn tableName="ranger_keystore"></dropColumn>-->
+ <!-- </rollback>-->
Review Comment:
[nitpick] There is a block of commented-out rollback code in the changeset. Consider either removing it if not needed or implementing proper rollback logic and documenting its purpose.
```suggestion
<rollback>
<!-- Drop constraints/indexes that reference the new column (if any) -->
<sql>ALTER TABLE ranger_keystore DROP CONSTRAINT IF EXISTS constraint_name;</sql>
<!-- Drop the added column -->
<dropColumn tableName="ranger_keystore" columnName="kms_encoded_value"/>
</rollback>
```
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097480021
##########
liquibase-database-upgrade/src/test/java/org/apache/ranger/db/upgrade/FileDBForTest.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileReader;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+public class FileDBForTest {
+ private FileDBForTest() {
+ }
+
+ public static void deleteFileIfExists(String filename) {
+ File file = new File(filename);
+ if (file.exists()) {
+ if (file.delete()) {
+ System.out.println("File deleted successfully: " + filename);
+ } else {
+ System.out.println("Failed to delete the file: " + filename);
+ }
+ } else {
+ System.out.println("File does not exist: " + filename);
+ }
+ }
+
+ public static boolean tagExists(String filePath, String tagName) {
+ createFileIfNotExists(filePath);
+ try (BufferedReader reader = new BufferedReader(new FileReader(filePath))) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ String[] changeLogRow = line.split(",");
+ if (changeLogRow[changeLogRow.length - 1].equals(tagName)) {
+ return true; // String found
+ }
+ }
+ } catch (IOException e) {
+ System.err.println("Error reading file: " + e.getMessage());
+ }
+ return false; // String not found
+ }
+
+ public static void tag(String filePath, String tagName) {
+ createFileIfNotExists(filePath);
+ if (isFileEmpty(filePath)) {
+ List<String> lines = new ArrayList<>();
+ lines.add(",,," + tagName);
+ writeLines(filePath, lines, true);
+ //writeLineToFile(filePath,",,,"+tagName);
Review Comment:
Remove dead code
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2194003979
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibasePropertiesFactory.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Lazy;
+import org.springframework.stereotype.Component;
+
+import java.util.Map;
+
+@Component
+public class LiquibasePropertiesFactory {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibasePropertiesFactory.class);
+
+ Map<String, IConfigProvider> configProviders;
+
+ @Autowired
+ public LiquibasePropertiesFactory(@Lazy Map<String, IConfigProvider> configProviders) {
+ this.configProviders = configProviders;
+ }
+
+ public String getUrl(String serviceName) {
Review Comment:
Thanks !!
Incorporated
##########
liquibase-database-upgrade/src/test/java/org/apache/ranger/db/upgrade/FileDBForTest.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileReader;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+public class FileDBForTest {
+ private FileDBForTest() {
+ }
+
+ public static void deleteFileIfExists(String filename) {
+ File file = new File(filename);
+ if (file.exists()) {
+ if (file.delete()) {
+ System.out.println("File deleted successfully: " + filename);
+ } else {
+ System.out.println("Failed to delete the file: " + filename);
+ }
+ } else {
+ System.out.println("File does not exist: " + filename);
+ }
+ }
+
+ public static boolean tagExists(String filePath, String tagName) {
+ createFileIfNotExists(filePath);
+ try (BufferedReader reader = new BufferedReader(new FileReader(filePath))) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ String[] changeLogRow = line.split(",");
+ if (changeLogRow[changeLogRow.length - 1].equals(tagName)) {
+ return true; // String found
+ }
+ }
+ } catch (IOException e) {
+ System.err.println("Error reading file: " + e.getMessage());
+ }
+ return false; // String not found
+ }
+
+ public static void tag(String filePath, String tagName) {
+ createFileIfNotExists(filePath);
+ if (isFileEmpty(filePath)) {
+ List<String> lines = new ArrayList<>();
+ lines.add(",,," + tagName);
+ writeLines(filePath, lines, true);
+ //writeLineToFile(filePath,",,,"+tagName);
Review Comment:
Addressed
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097459237
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibasePropertiesFactory.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Lazy;
+import org.springframework.stereotype.Component;
+
+import java.util.Map;
+
+@Component
+public class LiquibasePropertiesFactory {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibasePropertiesFactory.class);
+
+ Map<String, IConfigProvider> configProviders;
+
+ @Autowired
+ public LiquibasePropertiesFactory(@Lazy Map<String, IConfigProvider> configProviders) {
+ this.configProviders = configProviders;
+ }
+
+ public String getUrl(String serviceName) {
Review Comment:
Cleaner and more concise version
```
public String getUrl(String serviceName) {
IConfigProvider provider = configProviders.get(serviceName);
if (provider == null) {
LOG.error("ConfigProvider for service {} is null", serviceName);
return "";
}
return provider.getUrl();
}
```
Also same for below methods
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097383483
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseDBUtilsMain.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ranger.db.upgrade;
+
+import com.beust.jcommander.JCommander;
+import com.google.gson.Gson;
+import liquibase.Scope;
+import liquibase.command.CommandResults;
+import liquibase.resource.ClassLoaderResourceAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.bridge.SLF4JBridgeHandler;
+
+import java.util.HashMap;
+
+public class LiquibaseDBUtilsMain {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseDBUtilsMain.class);
+
+ private LiquibaseDBUtilsMain() {
+ }
+
+ public static void main(String[] argv) {
+ ArgsParserLiquibaseDBUtilsMain argsParserLiquibaseDBUtilsMain = new ArgsParserLiquibaseDBUtilsMain();
+
+ JCommander.newBuilder()
+ .addObject(argsParserLiquibaseDBUtilsMain)
+ .build()
+ .parse(argv);
+
+ String op = argsParserLiquibaseDBUtilsMain.opName;
+ String tagName = argsParserLiquibaseDBUtilsMain.tagName;
+ String serviceName = argsParserLiquibaseDBUtilsMain.serviceName;
+ ICommandDriver commandDriver = SpringContext.getBean(ICommandDriver.class);
+ if (commandDriver == null) {
+ throw new RuntimeException("commandDriver not found. Exiting!");
+ }
+ HashMap<String, Object> response = new HashMap<>();
+ response.put("op_status", Constants.RETURN_CODE_FAILURE);
+ try {
+ Scope.child(Scope.Attr.resourceAccessor, new ClassLoaderResourceAccessor(), () -> {
+ switch (op) {
+ case "isUpgradeComplete":
+ final String tagNamePostUpdate = Utils.getPostUpdateTagName(tagName);
+ CommandResults upgradeCompleteTagExistsResult = commandDriver.tagExists(serviceName, tagNamePostUpdate);
+ if (LiquibaseCommandResultsUtil.doesTagExist(upgradeCompleteTagExistsResult)) {
+ LOG.info("isUpgradeComplete={}", Constants.RETURN_CODE_SUCCESS);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isUpgradeComplete", Constants.RETURN_CODE_SUCCESS);
+ } else {
+ LOG.info("isUpgradeComplete={}", Constants.RETURN_CODE_FAILURE);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isUpgradeComplete", Constants.RETURN_CODE_FAILURE);
+ }
+ break;
+ case "isFinalizeComplete":
+ final String tagNameForFinalizeComplete = Utils.getFinalizeTagName(tagName);
+ CommandResults finalizeCompleteTagExistsResult = commandDriver.tagExists(serviceName, tagNameForFinalizeComplete);
+ if (LiquibaseCommandResultsUtil.doesTagExist(finalizeCompleteTagExistsResult)) {
+ LOG.info("isFinalizeComplete={}", Constants.RETURN_CODE_SUCCESS);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isFinalizeComplete", Constants.RETURN_CODE_SUCCESS);
+ } else {
+ LOG.info("isFinalizeComplete={}", Constants.RETURN_CODE_FAILURE);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isFinalizeComplete", Constants.RETURN_CODE_FAILURE);
+ }
+ break;
+// case "releaseLock":
Review Comment:
Can we shift this to TODO?
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2193995982
##########
liquibase-database-upgrade/changelogs/kms/db/postgres/create_trigger_kmsencoded_postgres.sql:
##########
@@ -0,0 +1,40 @@
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+--liquibase formatted sql
+
+-- Create a trigger function
+CREATE OR REPLACE FUNCTION copy_data_trigger_function()
+RETURNS TRIGGER AS
+'
+BEGIN
+ -- If column_a is updated and is not null, update column_b
+ IF NEW.kms_encoded IS NOT NULL THEN
+ NEW.kms_encoded_value := NEW.kms_encoded;
+ END IF;
+ -- If column_b is updated and is not null, update column_a
+ IF NEW.kms_encoded_value IS NOT NULL THEN
Review Comment:
Addressed
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097399071
##########
liquibase-database-upgrade/changelogs/kms/db/postgres/create_trigger_kmsencoded_postgres.sql:
##########
@@ -0,0 +1,40 @@
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+--liquibase formatted sql
+
+-- Create a trigger function
+CREATE OR REPLACE FUNCTION copy_data_trigger_function()
+RETURNS TRIGGER AS
+'
+BEGIN
+ -- If column_a is updated and is not null, update column_b
+ IF NEW.kms_encoded IS NOT NULL THEN
+ NEW.kms_encoded_value := NEW.kms_encoded;
+ END IF;
+ -- If column_b is updated and is not null, update column_a
+ IF NEW.kms_encoded_value IS NOT NULL THEN
Review Comment:
To avoid unintended overwrites and to make trigger logic more robust I think it will be better to check
-- If kms_encoded_value is set and kms_encoded is null, copy to kms_encoded
IF NEW.kms_encoded_value IS NOT NULL AND NEW.kms_encoded IS NULL THEN
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "vikaskr22 (via GitHub)" <gi...@apache.org>.
vikaskr22 commented on PR #569:
URL: https://github.com/apache/ranger/pull/569#issuecomment-3116762042
@fateh288 , thanks for the implementation.
Following is the review comments on design part, not on implementation.
As per one of the discussion with you and my understanding, consider following scenario:
There is a change in the format of the data in one particular column. Say Table1.Col1, as per current approach, during schema upgrade, a new column (say col2) of required type will be created and older data will be copied to new column after transformation. Now we may have two scenario:
1. New binaries not yet applied, means, read/write will continue to happen with old column. How you are handling the case, where a column is being updated by the application after copying from col1-> col2 ? If we are using a trigger on each data modification that will copy to new column. Then, we have multiple triggers for the same column ( if multiple modification is happening), and these triggers may also overlap with the job/cursor that is copying old format data to new column.
This scenario is posing two risks,
1. Triggers processing ( due to multiple updates on same column ) may be processed in different order and older data may be written to new column.
2. what if some trigger processing/execution fails, We may log this, report this. But do we have any systematic way ( out of box from the framework itself) to detect this and retry ?
I can think of one solution that may work but please let me know if it fits in the current approach:
Step1: Apply the schema changes, means, create a new column.
Step2: Using dynamic configuration (needs to be implemented), let running instances know that ZDU process has started and in such cases they will write to old column in older format and additionally they will also write an event in one table (say ZDU upgrade table, a new table). After writing at both places , then only, transaction will be successful.
Step3: As part of your framework, that contains logic to migrate data from col1 to col2, you code/cursor should read event in the insertion order to process this and once it is processed, then only it should be deleted from table. If any Runtime error occurs, since it has not been deleted, it will be retried.
This approach ensures, it would be processed in the order in which they occurred. And , out of bix from the framework, there would be way to know if all migration done or not.
We should also consider adding one step in "Finalisation Step", to check that this new event table should be empty.
This is just an idea, feel free to add your input if above scenario can be handled in different or better way.
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097411253
##########
liquibase-database-upgrade/changelogs/kms/db/test_3.x/sleep.xml:
##########
@@ -0,0 +1,37 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<databaseChangeLog
+ xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+ xmlns:pro="http://www.liquibase.org/xml/ns/pro"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+ http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd
+ http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd
+ http://www.liquibase.org/xml/ns/pro http://www.liquibase.org/xml/ns/pro/liquibase-pro-latest.xsd">
+
+ <changeSet id="6_sleep" author="fateh">
+ <sql dbms="postgresql" >
+ select pg_sleep(60);
+ </sql>
+ <sql dbms="mysql" >
Review Comment:
Thought out the implementation it seems we are working on postgressql and reason of adding mysql here?
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097439589
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseCommandResultsUtil.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import liquibase.command.CommandResults;
+import liquibase.report.OperationInfo;
+import liquibase.report.UpdateReportParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LiquibaseCommandResultsUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseCommandResultsUtil.class);
+
+ private LiquibaseCommandResultsUtil() {
+ }
+
+ public static boolean doesTagExist(CommandResults tagExistsResult) {
+ boolean res = (Boolean) tagExistsResult.getResult("tagExistsResult");
+ LOG.info("doesTagExist({})={}", tagExistsResult, res);
+ return res;
+ }
+
+ public static boolean isUpdateSuccessful(CommandResults updateResult) {
+ /*
+ TODO: revisit this
+ */
+ boolean operationInfoResult = updateResult.getResult("operationInfo") != null &&
+ ((OperationInfo) updateResult.getResult("operationInfo")).getOperationOutcome().equals("success");
Review Comment:
No type-checking before casting from Object to OperationInfo, which could lead to ClassCastException .
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2194003635
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseCommandResultsUtil.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import liquibase.command.CommandResults;
+import liquibase.report.OperationInfo;
+import liquibase.report.UpdateReportParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LiquibaseCommandResultsUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseCommandResultsUtil.class);
+
+ private LiquibaseCommandResultsUtil() {
+ }
+
+ public static boolean doesTagExist(CommandResults tagExistsResult) {
+ boolean res = (Boolean) tagExistsResult.getResult("tagExistsResult");
+ LOG.info("doesTagExist({})={}", tagExistsResult, res);
+ return res;
+ }
+
+ public static boolean isUpdateSuccessful(CommandResults updateResult) {
+ /*
+ TODO: revisit this
+ */
+ boolean operationInfoResult = updateResult.getResult("operationInfo") != null &&
+ ((OperationInfo) updateResult.getResult("operationInfo")).getOperationOutcome().equals("success");
Review Comment:
Addressed
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseCommandResultsUtil.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import liquibase.command.CommandResults;
+import liquibase.report.OperationInfo;
+import liquibase.report.UpdateReportParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LiquibaseCommandResultsUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseCommandResultsUtil.class);
+
+ private LiquibaseCommandResultsUtil() {
+ }
+
+ public static boolean doesTagExist(CommandResults tagExistsResult) {
+ boolean res = (Boolean) tagExistsResult.getResult("tagExistsResult");
+ LOG.info("doesTagExist({})={}", tagExistsResult, res);
+ return res;
+ }
+
+ public static boolean isUpdateSuccessful(CommandResults updateResult) {
+ /*
+ TODO: revisit this
+ */
+ boolean operationInfoResult = updateResult.getResult("operationInfo") != null &&
+ ((OperationInfo) updateResult.getResult("operationInfo")).getOperationOutcome().equals("success");
+ boolean updateReportResult = updateResult.getResult("updateReport") != null && (((UpdateReportParameters) updateResult.getResult("updateReport")).getSuccess());
Review Comment:
Addressed
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2194003420
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseCommandResultsUtil.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import liquibase.command.CommandResults;
+import liquibase.report.OperationInfo;
+import liquibase.report.UpdateReportParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LiquibaseCommandResultsUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseCommandResultsUtil.class);
+
+ private LiquibaseCommandResultsUtil() {
+ }
+
+ public static boolean doesTagExist(CommandResults tagExistsResult) {
+ boolean res = (Boolean) tagExistsResult.getResult("tagExistsResult");
+ LOG.info("doesTagExist({})={}", tagExistsResult, res);
+ return res;
+ }
+
+ public static boolean isUpdateSuccessful(CommandResults updateResult) {
+ /*
+ TODO: revisit this
+ */
+ boolean operationInfoResult = updateResult.getResult("operationInfo") != null &&
Review Comment:
@dhavalshah9131
If updateResult is indeed null somehow, I think it is ok that an exception is thrown.
I did give it a thought on whether we should return true or false by default if updateResult is null but it can go either ways and a wrong return value can result in incorrect logic being applied in the next steps which we dont want.
If there is a more elegant way to handle such scenarios I will be happy to incorporate the same. Let me know your suggestions
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097440515
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseCommandResultsUtil.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import liquibase.command.CommandResults;
+import liquibase.report.OperationInfo;
+import liquibase.report.UpdateReportParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LiquibaseCommandResultsUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseCommandResultsUtil.class);
+
+ private LiquibaseCommandResultsUtil() {
+ }
+
+ public static boolean doesTagExist(CommandResults tagExistsResult) {
+ boolean res = (Boolean) tagExistsResult.getResult("tagExistsResult");
+ LOG.info("doesTagExist({})={}", tagExistsResult, res);
+ return res;
+ }
+
+ public static boolean isUpdateSuccessful(CommandResults updateResult) {
+ /*
+ TODO: revisit this
+ */
+ boolean operationInfoResult = updateResult.getResult("operationInfo") != null &&
+ ((OperationInfo) updateResult.getResult("operationInfo")).getOperationOutcome().equals("success");
+ boolean updateReportResult = updateResult.getResult("updateReport") != null && (((UpdateReportParameters) updateResult.getResult("updateReport")).getSuccess());
Review Comment:
No type-checking before casting from Object to UpdateReportParameters, which could lead to ClassCastException .
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "vikaskr22 (via GitHub)" <gi...@apache.org>.
vikaskr22 commented on PR #569:
URL: https://github.com/apache/ranger/pull/569#issuecomment-3150346168
@fateh288 , I again went through the design and as explained by you as well, DB triggers processing happens as part of application transaction. Then in such cases, we don't need any separate mechanism to ensure data durability. So triggers getting executed out of order case is not valid. It should be taken care by the underlying DB. And hence the observation regrading triggers is INVALID, please ignore that.
Now let's see how we can address following scenario:
**Any update to old column after it has been copied but before trigger is created won't be updated in the new column.**
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on PR #569:
URL: https://github.com/apache/ranger/pull/569#issuecomment-3120882420
@vikaskr22
`New binaries not yet applied, means, read/write will continue to happen with old column. How you are handling the case, where a column is being updated by the application after copying from col1-> col2 ? `
--- great point. No, this case is not being handled - any update to old column after it has been copied but before trigger is created won't be updated in the new column. For KMS, it was observed that copy one column to another for 1M rows took 35 seconds. Realistically we won't have 1M rows and would just be a few hundred max rows so time between copy of column and trigger creation would be a few seconds max.
`If we are using a trigger on each data modification that will copy to new column. Then, we have multiple triggers for the same column ( if multiple modification is happening), and these triggers may also overlap with the job/cursor that is copying old format data to new column.`
--- Trigger is being created after the copy job is completed, so I am not clear how data copy job and trigger will overlap
```
This scenario is posing two risks,
Triggers processing ( due to multiple updates on same column ) may be processed in different order and older data may be written to new column.
```
--- from my understanding, is there are multiple modifications to same column, each of the triggers are processed sequentially - this behavior is out of the box for databases -- we can confirm this incase there is any gap in my understanding
`what if some trigger processing/execution fails, We may log this, report this. But do we have any systematic way ( out of box from the framework itself) to detect this and retry ?`
--- My understanding is that if the trigger fails, then the original transaction also fails i.e. original update will be reverted if the trigger fails to copy the data to the new column basically trigger is now a part of original transaction
```
Step1: Apply the schema changes, means, create a new column.
Step2: Using dynamic configuration (needs to be implemented), let running instances know that ZDU process has started and in such cases they will write to old column in older format and additionally they will also write an event in one table (say ZDU upgrade table, a new table). After writing at both places , then only, transaction will be successful.
Step3: As part of your framework, that contains logic to migrate data from col1 to col2, you code/cursor should read event in the insertion order to process this and once it is processed, then only it should be deleted from table. If any Runtime error occurs, since it has not been deleted, it will be retried.
```
--- I am not clear on this. The old bits / old kms instance will not have logic to write to a new table. Old application is not aware of new schema changes, new table that needs to be written to. Writing to both new and old columns can be done by new KMS instance so that new data is available to the old instances too. But how old instances can write to new column I don't fully understand.
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097397367
##########
liquibase-database-upgrade/changelogs/kms/db/postgres/create_trigger_kmsencoded_postgres.sql:
##########
@@ -0,0 +1,40 @@
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+--liquibase formatted sql
+
+-- Create a trigger function
+CREATE OR REPLACE FUNCTION copy_data_trigger_function()
+RETURNS TRIGGER AS
+'
+BEGIN
+ -- If column_a is updated and is not null, update column_b
+ IF NEW.kms_encoded IS NOT NULL THEN
Review Comment:
To avoid unintended overwrites and to make trigger logic more robust I think it will be better to check
-- If kms_encoded is set and kms_encoded_value is null, copy to kms_encoded_value
IF NEW.kms_encoded IS NOT NULL AND NEW.kms_encoded_value IS NULL THEN
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097432366
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseCommandResultsUtil.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import liquibase.command.CommandResults;
+import liquibase.report.OperationInfo;
+import liquibase.report.UpdateReportParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class LiquibaseCommandResultsUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseCommandResultsUtil.class);
+
+ private LiquibaseCommandResultsUtil() {
+ }
+
+ public static boolean doesTagExist(CommandResults tagExistsResult) {
+ boolean res = (Boolean) tagExistsResult.getResult("tagExistsResult");
+ LOG.info("doesTagExist({})={}", tagExistsResult, res);
+ return res;
+ }
+
+ public static boolean isUpdateSuccessful(CommandResults updateResult) {
+ /*
+ TODO: revisit this
+ */
+ boolean operationInfoResult = updateResult.getResult("operationInfo") != null &&
Review Comment:
No null check for updateResult.
Can raise Null pointer exception here.
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2193995793
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibaseDBUtilsMain.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ranger.db.upgrade;
+
+import com.beust.jcommander.JCommander;
+import com.google.gson.Gson;
+import liquibase.Scope;
+import liquibase.command.CommandResults;
+import liquibase.resource.ClassLoaderResourceAccessor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.bridge.SLF4JBridgeHandler;
+
+import java.util.HashMap;
+
+public class LiquibaseDBUtilsMain {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibaseDBUtilsMain.class);
+
+ private LiquibaseDBUtilsMain() {
+ }
+
+ public static void main(String[] argv) {
+ ArgsParserLiquibaseDBUtilsMain argsParserLiquibaseDBUtilsMain = new ArgsParserLiquibaseDBUtilsMain();
+
+ JCommander.newBuilder()
+ .addObject(argsParserLiquibaseDBUtilsMain)
+ .build()
+ .parse(argv);
+
+ String op = argsParserLiquibaseDBUtilsMain.opName;
+ String tagName = argsParserLiquibaseDBUtilsMain.tagName;
+ String serviceName = argsParserLiquibaseDBUtilsMain.serviceName;
+ ICommandDriver commandDriver = SpringContext.getBean(ICommandDriver.class);
+ if (commandDriver == null) {
+ throw new RuntimeException("commandDriver not found. Exiting!");
+ }
+ HashMap<String, Object> response = new HashMap<>();
+ response.put("op_status", Constants.RETURN_CODE_FAILURE);
+ try {
+ Scope.child(Scope.Attr.resourceAccessor, new ClassLoaderResourceAccessor(), () -> {
+ switch (op) {
+ case "isUpgradeComplete":
+ final String tagNamePostUpdate = Utils.getPostUpdateTagName(tagName);
+ CommandResults upgradeCompleteTagExistsResult = commandDriver.tagExists(serviceName, tagNamePostUpdate);
+ if (LiquibaseCommandResultsUtil.doesTagExist(upgradeCompleteTagExistsResult)) {
+ LOG.info("isUpgradeComplete={}", Constants.RETURN_CODE_SUCCESS);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isUpgradeComplete", Constants.RETURN_CODE_SUCCESS);
+ } else {
+ LOG.info("isUpgradeComplete={}", Constants.RETURN_CODE_FAILURE);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isUpgradeComplete", Constants.RETURN_CODE_FAILURE);
+ }
+ break;
+ case "isFinalizeComplete":
+ final String tagNameForFinalizeComplete = Utils.getFinalizeTagName(tagName);
+ CommandResults finalizeCompleteTagExistsResult = commandDriver.tagExists(serviceName, tagNameForFinalizeComplete);
+ if (LiquibaseCommandResultsUtil.doesTagExist(finalizeCompleteTagExistsResult)) {
+ LOG.info("isFinalizeComplete={}", Constants.RETURN_CODE_SUCCESS);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isFinalizeComplete", Constants.RETURN_CODE_SUCCESS);
+ } else {
+ LOG.info("isFinalizeComplete={}", Constants.RETURN_CODE_FAILURE);
+ response.put("op_status", Constants.RETURN_CODE_SUCCESS);
+ response.put("isFinalizeComplete", Constants.RETURN_CODE_FAILURE);
+ }
+ break;
+// case "releaseLock":
Review Comment:
Addressed
##########
liquibase-database-upgrade/changelogs/kms/db/postgres/create_trigger_kmsencoded_postgres.sql:
##########
@@ -0,0 +1,40 @@
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+--liquibase formatted sql
+
+-- Create a trigger function
+CREATE OR REPLACE FUNCTION copy_data_trigger_function()
+RETURNS TRIGGER AS
+'
+BEGIN
+ -- If column_a is updated and is not null, update column_b
+ IF NEW.kms_encoded IS NOT NULL THEN
Review Comment:
Addressed
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2194010382
##########
kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStoreProvider.java:
##########
@@ -66,7 +66,7 @@ public class RangerKeyStoreProvider extends KeyProvider {
private static final String CREDENTIAL_PATH = "ranger.ks.jpa.jdbc.credential.provider.path";
private static final String MK_CREDENTIAL_ALIAS = "ranger.ks.masterkey.credential.alias";
private static final String DB_CREDENTIAL_ALIAS = "ranger.ks.jpa.jdbc.credential.alias";
- private static final String DB_PASSWORD = "ranger.ks.jpa.jdbc.password";
+ public static final String DB_PASSWORD = "ranger.ks.jpa.jdbc.password";
Review Comment:
@dhavalshah9131 @vikaskr22 @spolavarapu1
Do you think this is a valid concern from copilot?
I don't think so but let me know your thoughts
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "fateh288 (via GitHub)" <gi...@apache.org>.
fateh288 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2193973234
##########
liquibase-database-upgrade/changelogs/kms/db/test_3.x/sleep.xml:
##########
@@ -0,0 +1,37 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<databaseChangeLog
+ xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+ xmlns:pro="http://www.liquibase.org/xml/ns/pro"
+ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+ http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-latest.xsd
+ http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd
+ http://www.liquibase.org/xml/ns/pro http://www.liquibase.org/xml/ns/pro/liquibase-pro-latest.xsd">
+
+ <changeSet id="6_sleep" author="fateh">
+ <sql dbms="postgresql" >
+ select pg_sleep(60);
+ </sql>
+ <sql dbms="mysql" >
Review Comment:
All the changesets/changelogs I have added in this PR are for demonstration purposes only to showcase what is possible in liquibase and the features it provides.
This particular changeset I am trying to demonstrate that a single changeset with a unique id and author can be used for multiple databases if we specify the dbms attribute.
Here sleep.xml has a changeset with changeSet id="6_sleep" author="fateh" which is used to specify behavior across multiple databases i.e. postgres as well as mysql.
This can drastically help reduce the number of changesets one has to keep track of since single changeset can now be used for multiple dbs
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5205: Liquibase based Zero-Downtime-Upgrade framework for Rang… [ranger]
Posted by "dhavalshah9131 (via GitHub)" <gi...@apache.org>.
dhavalshah9131 commented on code in PR #569:
URL: https://github.com/apache/ranger/pull/569#discussion_r2097459237
##########
liquibase-database-upgrade/src/main/java/org/apache/ranger/db/upgrade/LiquibasePropertiesFactory.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ranger.db.upgrade;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Lazy;
+import org.springframework.stereotype.Component;
+
+import java.util.Map;
+
+@Component
+public class LiquibasePropertiesFactory {
+ private static final Logger LOG = LoggerFactory.getLogger(LiquibasePropertiesFactory.class);
+
+ Map<String, IConfigProvider> configProviders;
+
+ @Autowired
+ public LiquibasePropertiesFactory(@Lazy Map<String, IConfigProvider> configProviders) {
+ this.configProviders = configProviders;
+ }
+
+ public String getUrl(String serviceName) {
Review Comment:
Cleaner and more concise version
`public String getUrl(String serviceName) {
IConfigProvider provider = configProviders.get(serviceName);
if (provider == null) {
LOG.error("ConfigProvider for service {} is null", serviceName);
return "";
}
return provider.getUrl();
}`
Also same for below methods
--
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-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org