Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 0.5
    • Component/s: Core
    • Labels:
      None
    • Environment:

      Cassandra Core Code

      Description

      Need a Cassandra Datacenter Quorum Read and Datacenter Quorum Write.

      Add 1 new enum DC_Quorum. basically reads with this will not span across the datacenter it will use the existing nodes in the Datacenter which has this data and read from it.
      For writes - All the data centers need to get this data, (datac enters will be configured in the storage-config.xml and number of replicas in it). Once configured write will basically write to all the nodes in all the datacenter but will wait only for the write in the current datacenter.
      Example: We have 3 Datacenter A,B,C A has a replication factor of 3, B has 2 and C has 2. DC_Quorum write will make sure to write on 2 of 3 nodes in A.... B and C (total 4 +1) nodes will be eventually consistent.

      Changes will be in RackAware, storage, read and write classes.

      Thanks
      Vijay

      1. cassandra-DCAWARE.patch
        7 kB
        Vijay
      2. cassandra-DCQuorum.patch
        27 kB
        Vijay
      3. DC-Config.xml
        0.8 kB
        Vijay

        Activity

        Vijay created issue -
        Vijay made changes -
        Field Original Value New Value
        Attachment cassandra-DCAWARE.patch [ 12422876 ]
        Hide
        Vijay added a comment -

        Configuration file to be placed @ /etc/cassandra/

        Show
        Vijay added a comment - Configuration file to be placed @ /etc/cassandra/
        Vijay made changes -
        Attachment DC-Config.xml [ 12422877 ]
        Hide
        Jonathan Ellis added a comment - - edited

        it is hard to see what is going on in the dcaware patch since there is so much reformatting of existing code going on. please resubmit without touching existing lines that doesn't actually need to be changed. (also, be aware of http://wiki.apache.org/cassandra/CodeStyle.)

        Show
        Jonathan Ellis added a comment - - edited it is hard to see what is going on in the dcaware patch since there is so much reformatting of existing code going on. please resubmit without touching existing lines that doesn't actually need to be changed. (also, be aware of http://wiki.apache.org/cassandra/CodeStyle .)
        Vijay made changes -
        Attachment cassandra-DCAWARE [ 12422920 ]
        Vijay made changes -
        Attachment cassandra-DCAWARE [ 12422920 ]
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12422921 ]
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12422876 ]
        Hide
        Jonathan Ellis added a comment - - edited

        this looks pretty good overall. quick work!

        some problems/feedback:

        EndPointSnitch, IEndPointSnitch, and CassandraServer hunks do not apply to trunk with patch -p0.

        we shouldn't need getMapReplicationFactor in IEndPointSnitch, and shouldn't need insertDCBlocking, this is too much abstraction leakage. i suggest moving determineBlockFor into AbstractReplicationStrategy, then the DC strategy version can use the configuration to determine blockFor w/o making EndPointSnitch/StorageProxy have to know about it. actually we probably need more than that, maybe a getQuorumResponseHandler(consistency_level) method and you can subclass QRH to know about the details of the DC layouts. Bottom line, StorageProxy shouldn't need to care about the details of the replicationstrategy.

        don't include boilerplate like this

        + * @param rm
        + * @param blockFor
        + * @param endpointMap
        + * @param primaryNodes
        + * @throws InvalidRequestException
        + * @throws UnavailableException
        + * @throws IOException
        + * @throws DigestMismatchException
        + * @throws TimeoutException

        real docstrings are fine but this doesn't add anything to what the method signature tells us

        there is still a lot of whitespace/formatting getting changed unnecessarily e.g. in StorageProxy.

        Show
        Jonathan Ellis added a comment - - edited this looks pretty good overall. quick work! some problems/feedback: EndPointSnitch, IEndPointSnitch, and CassandraServer hunks do not apply to trunk with patch -p0. we shouldn't need getMapReplicationFactor in IEndPointSnitch, and shouldn't need insertDCBlocking, this is too much abstraction leakage. i suggest moving determineBlockFor into AbstractReplicationStrategy, then the DC strategy version can use the configuration to determine blockFor w/o making EndPointSnitch/StorageProxy have to know about it. actually we probably need more than that, maybe a getQuorumResponseHandler(consistency_level) method and you can subclass QRH to know about the details of the DC layouts. Bottom line, StorageProxy shouldn't need to care about the details of the replicationstrategy. don't include boilerplate like this + * @param rm + * @param blockFor + * @param endpointMap + * @param primaryNodes + * @throws InvalidRequestException + * @throws UnavailableException + * @throws IOException + * @throws DigestMismatchException + * @throws TimeoutException real docstrings are fine but this doesn't add anything to what the method signature tells us there is still a lot of whitespace/formatting getting changed unnecessarily e.g. in StorageProxy.
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12422921 ]
        Hide
        Vijay added a comment -

        Updated and this uses QRH to achieve the functionality...

        Show
        Vijay added a comment - Updated and this uses QRH to achieve the functionality...
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423051 ]
        Hide
        Jonathan Ellis added a comment -

        Much better! The basic foundation is right now, just a little cleanup to do:

        the code in DataCenterShardSingleton belongs in DatacenterShardStategy

        DatacenterShardStategy should extend ARS and implement the abstract method, rather than overriding a non-abstract method for RUS. (otherwise, you break bootstrap.)

        DSS/DCSS shouldn't care about the bootstrap tokens, only the write methods in ARS need that and you shouldn't have to mess with those

        rename DCQuorumResponseHandlerSync to DCQuorumSyncResponseHandler for consistency

        change getNewQuorumRespone to
        public ResponseHandler getQuorumResponseHandler(IResponseResolver<Boolean> responseResolver, int consistency_level)

        "This Class has the dependency of DataCenterEndpointSnitch.class to be used." you should verify that with an assert

        use a 120 column width in formatting, not 80, and read through the code and fix obvious ugliness like
        + * @param endPoint
        + * the endPoint to process
        or
        + InetAddress endPointOfIntrest = tokenToEndPointMap.get(tokens
        + .get);

        goffinet, I think you had some comments about reloadConfiguration using properties files instead of xml?

        Show
        Jonathan Ellis added a comment - Much better! The basic foundation is right now, just a little cleanup to do: the code in DataCenterShardSingleton belongs in DatacenterShardStategy DatacenterShardStategy should extend ARS and implement the abstract method, rather than overriding a non-abstract method for RUS. (otherwise, you break bootstrap.) DSS/DCSS shouldn't care about the bootstrap tokens, only the write methods in ARS need that and you shouldn't have to mess with those rename DCQuorumResponseHandlerSync to DCQuorumSyncResponseHandler for consistency change getNewQuorumRespone to public ResponseHandler getQuorumResponseHandler(IResponseResolver<Boolean> responseResolver, int consistency_level) "This Class has the dependency of DataCenterEndpointSnitch.class to be used." you should verify that with an assert use a 120 column width in formatting, not 80, and read through the code and fix obvious ugliness like + * @param endPoint + * the endPoint to process or + InetAddress endPointOfIntrest = tokenToEndPointMap.get(tokens + .get ); goffinet, I think you had some comments about reloadConfiguration using properties files instead of xml?
        Jonathan Ellis made changes -
        Assignee Vijay [ vijay2win@yahoo.com ]
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423051 ]
        Hide
        Vijay added a comment -

        Ok fixed....

        Show
        Vijay added a comment - Ok fixed....
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423073 ]
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423073 ]
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423077 ]
        Hide
        Jonathan Ellis added a comment -

        Still seeing patch failures to EndPointSnitch, IEndPointSnitch, StorageProxy. can you update for trunk?

        Show
        Jonathan Ellis added a comment - Still seeing patch failures to EndPointSnitch, IEndPointSnitch, StorageProxy. can you update for trunk?
        Hide
        Vijay added a comment -

        I did use the trunk..... Renamed the file name...... Can you try this?

        Show
        Vijay added a comment - I did use the trunk..... Renamed the file name...... Can you try this?
        Vijay made changes -
        Attachment cassandra-DCQuorum.patch [ 12423132 ]
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423077 ]
        Hide
        Jonathan Ellis added a comment -

        same thing. did you svn up to resolve conflicts?

        Show
        Jonathan Ellis added a comment - same thing. did you svn up to resolve conflicts?
        Hide
        Vijay added a comment -

        Sorry about it... was trying it in the eclipse way, saw some conflicts and resolved it... Can you try this one?

        viparthaMacPro:cassandra vipartha$ svn up
        At revision 829621.
        viparthaMacPro:cassandra vipartha$

        Show
        Vijay added a comment - Sorry about it... was trying it in the eclipse way, saw some conflicts and resolved it... Can you try this one? viparthaMacPro:cassandra vipartha$ svn up At revision 829621. viparthaMacPro:cassandra vipartha$
        Vijay made changes -
        Attachment cassandra-DCQuorum.patch [ 12423148 ]
        Vijay made changes -
        Attachment cassandra-DCQuorum.patch [ 12423132 ]
        Hide
        Jonathan Ellis added a comment -

        Committed, with some changes:

        fixed use of generics in the new classes, especially wrt response handlers
        replaced tabs with spaces
        added new consistency levels to thrift
        made class naming more consistent

        There is one problem still to resolve; blockFor from StorageProxy is a problem. It needs to be determined by the replicationstrategy responsehandler only when it's useful, rather than being computed in StorageProxy (which doesn't have all the right information anyway, in the dc cases) and passed to getResponseHandler (where it's ignored except for one case).

        Show
        Jonathan Ellis added a comment - Committed, with some changes: fixed use of generics in the new classes, especially wrt response handlers replaced tabs with spaces added new consistency levels to thrift made class naming more consistent There is one problem still to resolve; blockFor from StorageProxy is a problem. It needs to be determined by the replicationstrategy responsehandler only when it's useful, rather than being computed in StorageProxy (which doesn't have all the right information anyway, in the dc cases) and passed to getResponseHandler (where it's ignored except for one case).
        Jonathan Ellis made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 0.4 [ 12313862 ]
        Fix Version/s 0.5 [ 12314040 ]
        Resolution Fixed [ 1 ]
        Jonathan Ellis made changes -
        Affects Version/s 0.5 [ 12314040 ]
        Fix Version/s 0.5 [ 12314040 ]
        Fix Version/s 0.4 [ 12313862 ]
        Hide
        Chris Goffinet added a comment -

        My only question was going to be the implementation for Endpoint Snitch. At Digg we wrote a custom snitch that reads from a properties file called rack.properties. Might we want to do the same thing for this?

        Show
        Chris Goffinet added a comment - My only question was going to be the implementation for Endpoint Snitch. At Digg we wrote a custom snitch that reads from a properties file called rack.properties. Might we want to do the same thing for this?
        Hide
        Jonathan Ellis added a comment -

        i'd rather see something properties based than xml too

        are you referring to the code in contrib/, Chris?

        Show
        Jonathan Ellis added a comment - i'd rather see something properties based than xml too are you referring to the code in contrib/, Chris?
        Hide
        Chris Goffinet added a comment -

        Yes

        Show
        Chris Goffinet added a comment - Yes
        Hide
        Hudson added a comment -

        Integrated in Cassandra #240 (See http://hudson.zones.apache.org/hudson/job/Cassandra/240/)
        add dcquorum/dcquorumsync consistency levels, representing "a quorum of the replicas in the current dc" and "a quorum of the replicas in each dc," respectively.
        patch by Vijay Parthasarathy; reviewed by jbellis for

        Show
        Hudson added a comment - Integrated in Cassandra #240 (See http://hudson.zones.apache.org/hudson/job/Cassandra/240/ ) add dcquorum/dcquorumsync consistency levels, representing "a quorum of the replicas in the current dc" and "a quorum of the replicas in each dc," respectively. patch by Vijay Parthasarathy; reviewed by jbellis for
        Hide
        Vijay added a comment -

        Added the fix as DC quorum in the trunk doesnt work... there is also a performance improvement along with this.

        Thanks

        Show
        Vijay added a comment - Added the fix as DC quorum in the trunk doesnt work... there is also a performance improvement along with this. Thanks
        Vijay made changes -
        Attachment cassandra-DCAWARE.patch [ 12423666 ]
        Hide
        Jonathan Ellis added a comment -

        committed with minor fixes (stop using tabs for indentation pls)

        Show
        Jonathan Ellis added a comment - committed with minor fixes (stop using tabs for indentation pls)
        Hide
        Hudson added a comment -

        Integrated in Cassandra #244 (See http://hudson.zones.apache.org/hudson/job/Cassandra/244/)
        fixes for DQ quorum code. patch by Vijay Parthasarathy; reviewed by jbellis for

        Show
        Hudson added a comment - Integrated in Cassandra #244 (See http://hudson.zones.apache.org/hudson/job/Cassandra/244/ ) fixes for DQ quorum code. patch by Vijay Parthasarathy; reviewed by jbellis for
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12479520 ] patch-available, re-open possible [ 12751972 ]
        Gavin made changes -
        Workflow patch-available, re-open possible [ 12751972 ] reopen-resolved, no closed status, patch-avail, testing [ 12758049 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        11d 9h 48m 1 Jonathan Ellis 26/Oct/09 15:08

          People

          • Assignee:
            Vijay
            Reporter:
            Vijay
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 336h
              336h
              Remaining:
              Remaining Estimate - 336h
              336h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development