dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.07k stars 2.03k forks source link

Oracle support for storage and system data storage #3561

Closed eisendle closed 2 years ago

eisendle commented 6 years ago

I´m currently working on support for Oracle DB as grain state storage and system data store (Membership, Reminders, Metrics,...). After finishing the script I discovered several issues with AdoNetStorageProvider. OPD.net is in several areas not compatible with the present AdoNetStorageProvider. I´m trying not rant about OPD.net here ;) The purpose of this issue is to discuss possible fixes for these issues. After a go from your side, I will supply several pull requests to achieve support for Oracle DB.

I´ve discovered following issues so far:

veikkoeeva commented 6 years ago

@eisendle The Oracle support has been non-existent, but those are roughly the problems one ends up with, I believe. One other thing I can think of is that statistics are inserted in batches (see here and Oracle is the odd one with partically no testing against a real DB).

The other problems in order:

1) ODP.NET dll loading. This should be fixed, naturally. 2) Maybe just remove the semicolon from the query? Every other DB should consume without a hiccup, I believe (maybe there's a configuration know, but by default all DBs probably consume queries without semicolons). 3) Those are read only once and are, in fact, partially dependent on dll version. Some versions of Npsql, for instance, have a problem with asynchronous loading, others don't. Indeed, something could be figured out later, separately. Opinions are appreciated. 👍 4) I just checked the data type mapping (here and you are right about it. SO proposes hacking the ADO.NET client, Oracle has a note about custom integer mappings that might help.

eisendle commented 6 years ago

@veikkoeeva

ad 1 I don´t see a problem here, because all you have to do, is to add the Oracle.ManagedDataAccess nuget package to your server project and make sure the DLL is deployed.

ad 2 Removing the semicolon may work for the currently supported databases, but I would prefer my proposed solution, because maybe when adding support for another DBS the problem would rise again. Also the testing effort is larger, because the semicolon less queries have to be tested with all supported databases.

ad 3 I don´t get your point here. The current if statements which determine support for async loading are neither provider version aware.... Maybe the tuple in DbConnectionFactory could be extended with a minimum version supported by orleans.

ad 4 I´ve found out why GetValue fails: GetValue() tries to cast the decimal db value to int, which fails because it´s a reference of object. My suggestion here would be to check whether the supplied value from the DbReader is of the type int. If its a int to the cast and if not use Convert.ToInt32. This is quite a hack, but not so ugly like the workaround you mentioned on SO. The custom integer mappings work only for entity framework not for DBDataReader, so this is a no go.

veikkoeeva commented 6 years ago

@eisendle

  1. Indeed. It looks to me you need to chance the dll name to match what's provided in the Oracle package.
  2. Sorry, I was unclear about why I suggested removing the semicolon: check without to make quick progress. The reasons I note is that it works with a semicolon in SQL Server, MySQL and PostgreSQL and in the Internet I see Oracle examples use it too. I don't have access to Oracle until next week, but I can ask some colleagues at work tomorrow about their opinion.
  3. The provider is checked as in doNetInvariants.InvariantNameMySql and the result is stored here. So, it checks the same string that is used to load the library. Currenty the version of the library is not checked as the measures taken doesn't do anything wrong (there's a performance issue, neglible mostly, for starting a new Task, for instance) and since such a check would be messy anyhow, but possible in principle.
  4. I corroborate your finding about Oracle client casting integer from the DB to decimal. I don't use much Oracle, but I can confirm this amongst the sources I linked. I think the way of checking the type is OK, perhaps also checking if the client is Oracle and assuming it by default tries to cast to decimal. I would be inclined to believe it's the underlying Oracle ADO.NET client that does the mapping even if in the documents they mention Entity Framework. It could also be a worth a try, but simultaneously I mention checking the type is probably the least hassle and most robust way of doing it. So, all in all, after enumerating the choices, we perhaps choose that one and maybe even provide reasoning along these lines in the comments for the cast so it's not lost.
eisendle commented 6 years ago

@veikkoeeva

ad 1. Already done in the pull request

ad 2.

I took the time to check whether omitting the semicolon makes problem on the other supported DBSes. Looks good, no other data provider makes problems with semicolon less statements. Therefore I will make a commit which removes the semicolon from the OrleansQueries read queries. You mentioned that you have seen Oracle queries with semicolons on the internet. What you have probably seen are PL/SQL statements which are required to end with a semicolon. See here.

ad 3. I´m afraid we are talking past each other ;). I will add the changes to the pull request and then let´s see what think about it.

ad 4. For the decimal problem I would stick to the simple cast solution. I will add a comment which explains the circumstances and link to this issue.

sergeybykov commented 6 years ago

@eisendle @veikkoeeva Do we need to do something here, especially for 2.0.0?

ghost commented 2 years ago

We are marking this issue as stale due to the lack of activity in the past six months. If there is no further activity within two weeks, this issue will be closed. You can always create a new issue based on the guidelines provided in our pinned announcement.

ghost commented 2 years ago

This issue has been marked stale for the past 30 and is being closed due to lack of activity.