add isolation level to Oracle Plugin: PLUGIN-1178 by jster1357 · Pull Request #258 · data-integrations/database-plugins

jster1357 · 2022-04-29T21:05:00Z

Oracle specific changes. I'm going to work on the broader set of databases.

https://cdap.atlassian.net/browse/PLUGIN-1178

jster1357 · 2022-04-29T21:06:03Z

I wanted to get an opinion on if we even needed an isolation level setting for a source plugin. Furthermore, would it make sense for an action plugin. It certainly makes sense for a sink plugin but I'm on the fence on Action and wether we need it for source.

itsankit-google · 2022-05-03T16:43:05Z

+    @Description("The transaction isolation level for the database session.")
+    @Nullable
+    @Macro
+    public String transactionIsolationLevel;


I think we can move this property to OracleConnectorConfig instead of independently in sink and source.

+1, we'd better add more descriptions here suggesting what transaction level to set for corresponding role. As I recall, you can not set arbitrary isolation level for sysdba or sysopr role, that's why we have https://github.com/data-integrations/database-plugins/blob/develop/oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConnectorConfig.java#L109

also since isolation level is a common connection property , if we are only focusing on Oracle now, let's add a TODO to move it to AbstractDBSpecificConnectorConfig/AbstractDBConnectorConfig in the future

@seanzhougoogle I kept the isolation level changes just to the Oracle Plugin. I'm currently working on the broader change across the other DB's and I'm using AbstractDBSpecificConnectorConfig/AbstractDBConnectorConfig for those

@seanzhougoogle I can confirm on the sysdba user failing w/ serialized isolation levels. I recieve ORA-08178: illegal SERIALIZABLE clause specified for user INTERNAL

We should document this in the docs (plugin) that any selection w/ an internal user like sysdba or sysop will result in the isolation level being changed to read_committed.

@jster1357 Could you please check if its possible to not display transaction_serializable in UI if sysdba or sysop is selected?

@itsankit-google got it. I'm looking into using a filter to hide the transaction leves when sysop or sysdba is selected

itsankit-google

Is there any specific reason to leave out TRANSACTION_READ_UNCOMMITTED & TRANSACTION_REPEATABLE_READ? In generic DB plugin, we do allow configuring them as well.

jster1357 · 2022-05-03T19:07:03Z

I only exposed those specific isolation levels because Oracle doesn't support the other 2 levels. I had it originally but had errors when using REPEATED and UNCOMMITTED levels. I thought this approach would help save the user from themselves so to speak.

jster1357 · 2022-05-05T01:52:42Z

I made the change from having the transaction level code at the source/sink level to the ConnectorConfig Class. Things work with the exception of when the roles are set for SYSDBA or SYSOP. I can run fine using the Normal role and I can get the isolation level to change just fine. When I have a SYSDBA role selected, I always get ORA-08178: illegal SERIALIZABLE clause specified for user INTERNAL even though I have logic to remap the isolation level when I see that role being used. It always grabs the serialized...I must be missing something simple here. I tired overriding the getTransactionIsolationLevel method but it's still not picking up the changes.

I also made the change to hide the isolation field when a user selects the SYSDBA or SYSOPS radio button.

itsankit-google

Some comments.

itsankit-google · 2022-05-06T07:02:47Z

I made the change from having the transaction level code at the source/sink level to the ConnectorConfig Class. Things work with the exception of when the roles are set for SYSDBA or SYSOP. I can run fine using the Normal role and I can get the isolation level to change just fine. When I have a SYSDBA role selected, I always get ORA-08178: illegal SERIALIZABLE clause specified for user INTERNAL even though I have logic to remap the isolation level when I see that role being used. It always grabs the serialized...I must be missing something simple here. I tired overriding the getTransactionIsolationLevel method but it's still not picking up the changes.

I also made the change to hide the isolation field when a user selects the SYSDBA or SYSOPS radio button.

I think it was because of removal of getTransactionIsolationLevel() overridden methods from OracleSourceConfig and OracleSinkConfig which made it always return null and setting the default TRANSACTION_SERIALIZABLE isolation level.

jster1357 · 2022-05-06T18:21:33Z

I've made the changes and everything looks good on my end. I've tested the following situations:

Normal Role with null isolation level, job runs with SERIALIZABLE
Normal Role with SERIALIZABLE Level, job runs with SERIALIZABLE
Normal Role with READ_COMMITTED level, job runs with READ_COMMITTED
SYSDBA Role, UI Hides isolation level selection, job runs with READ_COMMITTED
import pipeline with SYSDBA role and SERIALIZABLE level, job runs with READ_COMMITTED

I don't think we need to expose the isolation level w/ a predefined connector...that should be done on a table by table basis.

itsankit-google · 2022-05-06T18:28:27Z

I don't think we need to expose the isolation level w/ a predefined connector...that should be done on a table by table basis.

Predefined connections also act like a source for sampling the tables through wrangler.
@seanzhougoogle WDYT?

itsankit-google · 2022-05-06T18:30:57Z

      ]
    }
  ],
+  "Filters" : [],


Instead of empty, we need to add a similar filter for isolation levels as in source and sink.

I added the filter but I don't see the ISOLATION LEVEL showing up. I didn't even see it prior to adding the filter, which was why I originally left it off.

yeah looks like there is some issue in the UI not rendering it correctly? Is it only with select widget? Did you try changing the widget type?

I was able to verify your changes, looks like it is a UI bug when there are 2 connectors, one in system scope and other in user scope, UI only renders system scope one.

I tired the radio button but it didn't render. I thought the filter capitalization error would fix it but no dice.

Opened a JIRA for UI team - https://cdap.atlassian.net/browse/PLUGIN-1248

itsankit-google · 2022-05-06T19:49:53Z

LGTM, one minor comment. Thanks Justin :)

itsankit-google · 2022-05-06T20:28:58Z

I don't think we need to expose the isolation level w/ a predefined connector...that should be done on a table by table basis.

Predefined connections also act like a source for sampling the tables through wrangler. @seanzhougoogle WDYT?

Discussed offline with @seanzhougoogle, will need to live with this limitation because it is a connection property.

itsankit-google reviewed May 3, 2022

View reviewed changes