[jira] Issue Comment Edited: (CASSANDRA-734) Table.open has a broken lock in it

["Jeff Hodges (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-734?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12804225#action_12804225 ] 

Jeff Hodges edited comment on CASSANDRA-734 at 1/24/10 10:59 AM:
-----------------------------------------------------------------

To start with, synchronization being slow is mostly a scary story left around from the bad old days of Java 1.3 and lower. http://www.ibm.com/developerworks/java/library/j-jtp04223.html

Second, any thing we build instead of using synchronized will be nearly exactly duplicating synchronized's behavior except broken and slower. There seems to be nothing that compiles down to just "lock around this get and, if that's null, create this other thing and then put it in there" in Java bytecode. The only other way to make this work is load all the tables at boot time. Which, of course, is a non-starter. However, synchronized does say "all this work has to be done together" which fixes our bug and has years of JVM hackers behind it making it as fast as possible. 

Third, we should be aiming as much for correctness as we can. A Cassandra node is eventually consistent, but its codebase is not. Fixing a bug that will, eventually, kick Twitter and Digg and Rackspace's ass now is better than holding off until a "faster" way can be found in some possible future where unicorns live and candy mountains are not just scary things in creepy guys basements.

synchronized does a bang up job of fixing this bug now and doing so in a way that is more performant than other "correct" ways.

After this patch goes in, we should be re-evaluate all of these calls to Table.open(), though. I'm going to bet that in most cases it would make more sense for the client object to hold on to a reference to the Table if they need it and not let go every time like they do currently after the Table.open() call goes out of scope.

Edited for a friggin' "it's" grammar problem.

      was (Author: jmhodges):
    To start with, synchronization being slow is mostly a scary story left around from the bad old days of Java 1.3 and lower. http://www.ibm.com/developerworks/java/library/j-jtp04223.html

Second, any thing we build instead of using synchronized will be nearly exactly duplicating synchronized's behavior except broken and slower. There seems to be nothing that compiles down to just "lock around this get and, if that's null, create this other thing and then put it in there" in Java bytecode. The only other way to make this work is load all the tables at boot time. Which, of course, is a non-starter. However, synchronized does say "all this work has to be done together" which fixes our bug and has years of JVM hackers behind it making it as fast as possible. 

Third, we should be aiming as much for correctness as we can. A Cassandra node is eventually consistent, but it's codebase is not. Fixing a bug that will, eventually, kick Twitter and Digg and Rackspace's ass now is better than holding off until a "faster" way can be found in some possible future where unicorns live and candy mountains are not just scary things in creepy guys basements.

synchronized does a bang up job of fixing this bug now and doing so in a way that is more performant than other "correct" ways.

After this patch goes in, we should be re-evaluate all of these calls to Table.open(), though. I'm going to bet that in most cases it would make more sense for the client object to hold on to a reference to the Table if they need it and not let go every time like they do currently after the Table.open() call goes out of scope. 
  
> Table.open has a broken lock in it
> ----------------------------------
>
>                 Key: CASSANDRA-734
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-734
>             Project: Cassandra
>          Issue Type: Bug
>    Affects Versions: 0.5
>            Reporter: Jeff Hodges
>            Assignee: Jeff Hodges
>            Priority: Minor
>             Fix For: 0.6
>
>         Attachments: broken_lock_in_table_open.patch
>
>
> Table.open's lock is used around the Map#put method call but not the #get. This makes it a source of spurious bugs. The attached patch synchronizes the entire Table.open method and removes the unused createLock static.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.