[GitHub] [shardingsphere] terrymanu commented on a change in pull request #15566: Add support for generate narayana config file when the first resource added on cluster mode

[GitBox <gi...@apache.org>]

terrymanu commented on a change in pull request #15566:
URL: https://github.com/apache/shardingsphere/pull/15566#discussion_r812558480



##########
File path: shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/spi/TransactionConfigurationFileGenerator.java
##########
@@ -41,9 +41,10 @@
     /**
      * Get transaction configuration.
      *
-     * @param transactionRuleConfiguration transaction rule configuration
+     * @param originTransactionProps origin transaction properties
      * @param schemaConfiguration schema configuration
+     * @param modeType mode type
      * @return transaction rule props
      */
-    Properties getTransactionProps(TransactionRuleConfiguration transactionRuleConfiguration, SchemaConfiguration schemaConfiguration);
+    Optional<Properties> getTransactionProps(Properties originTransactionProps, Optional<SchemaConfiguration> schemaConfiguration, String modeType);

Review comment:
       Input param cannot permit optional value, please follow IDEA's warning to adjust it.

##########
File path: shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/spi/TransactionConfigurationFileGenerator.java
##########
@@ -41,9 +41,10 @@
     /**
      * Get transaction configuration.
      *
-     * @param transactionRuleConfiguration transaction rule configuration
+     * @param originTransactionProps origin transaction properties
      * @param schemaConfiguration schema configuration
+     * @param modeType mode type
      * @return transaction rule props

Review comment:
       The java doc is `Get transaction configuration`, but the return value is `transaction rule props`, please unify them

##########
File path: shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-type/shardingsphere-transaction-xa/shardingsphere-transaction-xa-provider/shardingsphere-transaction-xa-narayana/src/main/java/org/apache/shardingsphere/transaction/xa/narayana/config/NarayanaConfigurationFileGenerator.java
##########
@@ -139,21 +138,21 @@ private void appendJdbcStoreConfiguration(final String jdbcUrl, final String use
     }
     
     @Override
-    public Properties getTransactionProps(final TransactionRuleConfiguration transactionRuleConfiguration, final SchemaConfiguration schemaConfiguration) {
+    public Optional<Properties> getTransactionProps(final Properties originTransactionProps, final Optional<SchemaConfiguration> schemaConfiguration, final String modeType) {
         Properties result = new Properties();
-        if (!transactionRuleConfiguration.getProps().isEmpty()) {
-            generateUserDefinedJdbcStoreConfiguration(transactionRuleConfiguration, result);
-        } else {
-            generateDefaultJdbcStoreConfiguration(schemaConfiguration, result);
+        if (!originTransactionProps.isEmpty()) {
+            generateUserDefinedJdbcStoreConfiguration(originTransactionProps, result);
+        } else if ("Cluster".equalsIgnoreCase(modeType) && schemaConfiguration.isPresent()) {

Review comment:
       `equalsIgnoreCase` or just `equals`? Does other codes use `equalsIgnoreCase` to judge mode type?

##########
File path: shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-type/shardingsphere-transaction-xa/shardingsphere-transaction-xa-provider/shardingsphere-transaction-xa-narayana/src/main/java/org/apache/shardingsphere/transaction/xa/narayana/config/NarayanaConfigurationFileGenerator.java
##########
@@ -57,7 +56,7 @@ public void generateFile(final TransactionRule transactionRule, final InstanceCo
         String instanceId = instanceContext.getInstance().getInstanceDefinition().getInstanceId().getId();
         String recoveryId = null == instanceContext.getInstance().getXaRecoveryId() ? instanceId : instanceContext.getInstance().getXaRecoveryId();
         NarayanaConfiguration config = createDefaultConfiguration(instanceId, recoveryId);
-        if (null != transactionRule.getProps()) {
+        if (null != transactionRule.getProps() && !transactionRule.getProps().isEmpty()) {

Review comment:
       When `transactionRule.getProps()` is null?

##########
File path: shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/spi/TransactionConfigurationFileGenerator.java
##########
@@ -41,9 +41,10 @@
     /**
      * Get transaction configuration.
      *
-     * @param transactionRuleConfiguration transaction rule configuration
+     * @param originTransactionProps origin transaction properties
      * @param schemaConfiguration schema configuration
+     * @param modeType mode type
      * @return transaction rule props
      */
-    Properties getTransactionProps(TransactionRuleConfiguration transactionRuleConfiguration, SchemaConfiguration schemaConfiguration);
+    Optional<Properties> getTransactionProps(Properties originTransactionProps, Optional<SchemaConfiguration> schemaConfiguration, String modeType);

Review comment:
       How about replace `Optional<Properties>` to empty Properties if no contents?




-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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