Enable WriteSerializable Isolation Level #1262
Conversation
…nSuite for WriteSerializable isolation level
|
this pullreq is WIP because i am waiting to hear if there is any good reason this should not be exposed... |
| Serializable.toString, | ||
| IsolationLevel.fromString(_), | ||
| _ => true, | ||
| "needs to be Serializable or WriteSerializable" |
There was a problem hiding this comment.
Since we don't have the implementation for WriteSerializable yet, could you make this config only accept Serializable?
We can target this PR to allow users to set delta.isolationLevel to Serializable. This is correct and there is no point to block people.
Supporting WriteSerializable can be a separate PR that would require a large change.
There was a problem hiding this comment.
can you explain what is missing for the implementation for WriteSerializable? that's what i am trying to understand.
what i can see is that the logic for WriteSerializable is already present in ConflictChecker and OptimisticTransaction, but i am unsure what else would be needed.
There was a problem hiding this comment.
I just took a quick look at the existing code for WriteSerializable. It might not be correct. I need to take a detail look and think through it. Haven't read these codes for a while. This is the most complicated part in Delta.
Meanwhile, you can change the code to enable Serializable only. Will open it to allow WriteSerializable when we fix the bugs.
There was a problem hiding this comment.
i did an experiment where i changed default isolation level to WriteSerializable and ran all the tests. the only tests that failed where the ones that should fail due to the change in behavior.
we also started testing multiple jobs writing concurrently to same table using WriteSerializable isolation level on real cluster and everything is behaving as expected.
i am in no way saying the code for WriteSerializable is correct, but if its not i haven't spotted the issues so far.
just FYI
|
ok i will create new pullreq to just expose isolation level and only allow Serializable. |
koertkuipers
changed the title
…alizable and add more tests
core/src/main/scala/org/apache/spark/sql/delta/DeltaConfig.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/delta/DeltaConfig.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/sql/delta/DeltaConfigSuite.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/sql/delta/DeltaConfigSuite.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/delta/DeltaConfig.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala
Outdated
Show resolved
Hide resolved
What's the status here? Did you create that new PR? Can we close this one? Thanks! |
koertkuipers
changed the title
the pullrequest to enable just setting Serializable isolation level has been merged. it fixes #1265 this pullrequest enables WriteSerializable isolation level, which is a separate issue that has not been resolved yet. see #1261 |
|
@zsxwing can you take a look please? this passes all tests without issues, and we have been running this for years now on clusters (HDFS and S3). the logic for
|
|
Is this still a WIP? |
i think it is not. let me change title. i does still need to be reviewed though by people that understand this part of codebase. |
koertkuipers
changed the title
|
Is there a plan to merge this ? |
|
When will this be made available for non-databricks users? Thought deltalake was meant to be open source, not making parts of the code non accessible by open source users/systems. |
Description
Resolves #1261
How was this patch tested?
Added tests for isolation level WriteSerializable in OptimisticTransactionSuite
Does this PR introduce any user-facing changes?
Yes. it enabled setting config
delta.isolationLevel=WriteSerializable