Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-4834

TAP mutation flags are in the wrong order

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.5.4, 1.7.2, 1.8.0
    • Component/s: None
    • Security Level: Public
    • Labels:
      None

      Description

      When recently working with a TAP client, it was discovered that the item flags are in the wrong order. Further investigation showed, at the time, that we store the item flags in network byte order, and then convert them to network byte order (again) before sending.

      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        ingenthr Matt Ingenthron added a comment -

        There was a discussion between Trond, Dustin and I on this earlier today. The discussion wasn't conclusive, but addressing this issue pretty much comes down to two options:

        1) Find a bit somewhere (tap flags?) to indicate whether or not item flags are in the correct order. This would allow us to, in a future release, turn on this bit and correct flag ordering when all nodes of the cluster are known to handle it. It would also allow us to immediately workaround in the issue in any TAP clients by reordering now (on the assumption that it's wrong, since we support only one architecture at the moment), and not reordering if the bit indicates the response is now correct.

        2) Document that item flags in TAP will be sent in the wrong order from certain architectures and that clients should reorder.

        Option number two isn't so nice in that it complicates ever-so-slightly client implementation, but more importantly makes it impossible to do TAP between different architectures whether server to client or between servers.

        Trond also mentioned that fixing it isn't exactly easy, as it's memcached that converts and it's not possible to know what to change at that level. The fix there would be to ntohl() that part before returning it to memcached, but then it'll be converted again. This makes bytes correct, but would be a bit confusing in the code and not exactly the best way to handle them.

        If possible, I'd like to get a good answer on this as there are updates needed very, very soon on the Java client to work with this. The requirement is that what we ship work with 1.8.0, so I know I'll need to reorder in the client for now, but would like to put it together in light of whatever long term approach we decide upon.

        See also http://review.couchbase.org/13510 which has a change for the Java client to do the mentioned reordering and further discussion.

        Show
        ingenthr Matt Ingenthron added a comment - There was a discussion between Trond, Dustin and I on this earlier today. The discussion wasn't conclusive, but addressing this issue pretty much comes down to two options: 1) Find a bit somewhere (tap flags?) to indicate whether or not item flags are in the correct order. This would allow us to, in a future release, turn on this bit and correct flag ordering when all nodes of the cluster are known to handle it. It would also allow us to immediately workaround in the issue in any TAP clients by reordering now (on the assumption that it's wrong, since we support only one architecture at the moment), and not reordering if the bit indicates the response is now correct. 2) Document that item flags in TAP will be sent in the wrong order from certain architectures and that clients should reorder. Option number two isn't so nice in that it complicates ever-so-slightly client implementation, but more importantly makes it impossible to do TAP between different architectures whether server to client or between servers. Trond also mentioned that fixing it isn't exactly easy, as it's memcached that converts and it's not possible to know what to change at that level. The fix there would be to ntohl() that part before returning it to memcached, but then it'll be converted again. This makes bytes correct, but would be a bit confusing in the code and not exactly the best way to handle them. If possible, I'd like to get a good answer on this as there are updates needed very, very soon on the Java client to work with this. The requirement is that what we ship work with 1.8.0, so I know I'll need to reorder in the client for now, but would like to put it together in light of whatever long term approach we decide upon. See also http://review.couchbase.org/13510 which has a change for the Java client to do the mentioned reordering and further discussion.
        Hide
        trond Trond Norbye added a comment -

        I assume that the best fix going forward is just to document that the flags are sent in little endian order over the tap connection unless the bit is set.. We don't have any bigendian deployments out there, so we don't have an upgrade scenario to take care of there

        Show
        trond Trond Norbye added a comment - I assume that the best fix going forward is just to document that the flags are sent in little endian order over the tap connection unless the bit is set.. We don't have any bigendian deployments out there, so we don't have an upgrade scenario to take care of there
        Hide
        ingenthr Matt Ingenthron added a comment -

        I agree with this approach. It'd need to be very, very well tested for upgrades, but it seems like the right thing to do.

        If we can identify the flag we'll eventually use, I'll be unblocked on the hadoop connector.

        Show
        ingenthr Matt Ingenthron added a comment - I agree with this approach. It'd need to be very, very well tested for upgrades, but it seems like the right thing to do. If we can identify the flag we'll eventually use, I'll be unblocked on the hadoop connector.
        Hide
        trond Trond Norbye added a comment -

        (Reduce priority to major.. this doesn't affect the data integrity inside the cluster, nor does it affect the cluster state (crashes/memory leaks))

        Show
        trond Trond Norbye added a comment - (Reduce priority to major.. this doesn't affect the data integrity inside the cluster, nor does it affect the cluster state (crashes/memory leaks))
        Show
        ingenthr Matt Ingenthron added a comment - Proposed fix: http://review.couchbase.org/13717 http://review.couchbase.org/13718
        Hide
        ingenthr Matt Ingenthron added a comment -

        I looked over the proposed fixes, and if possible I want to request a change.

        It would be good if when establishing the TAP stream the client could signal (via a flag) that they want things correct in network byte order. If the server has the fix, it'll order things correctly and set an appropriate flag.

        This way, the logic is pretty simple in the client. Set the flag, if not receiving a corresponding flag saying data is correct, then the client knows to reorder. The flag would be ignored by servers that don't know to fix their bytes.

        Then, in the future, when all supported servers are fixed, we can repurpose the flags for something else.

        Show
        ingenthr Matt Ingenthron added a comment - I looked over the proposed fixes, and if possible I want to request a change. It would be good if when establishing the TAP stream the client could signal (via a flag) that they want things correct in network byte order. If the server has the fix, it'll order things correctly and set an appropriate flag. This way, the logic is pretty simple in the client. Set the flag, if not receiving a corresponding flag saying data is correct, then the client knows to reorder. The flag would be ignored by servers that don't know to fix their bytes. Then, in the future, when all supported servers are fixed, we can repurpose the flags for something else.
        Hide
        trond Trond Norbye added a comment -

        How does that differ from the implementation in the patches?

        Show
        trond Trond Norbye added a comment - How does that differ from the implementation in the patches?
        Hide
        ingenthr Matt Ingenthron added a comment -

        Maybe it doesn't and I misread them. Sorry.

        Upon rereading, it looks like a client would set TAP_CONNECT_TAP_FIX_FLAG_BYTEORDER and then in the received mutations, we'll have TAP_FLAG_NETWORK_BYTE_ORDER set. Thanks!

        Show
        ingenthr Matt Ingenthron added a comment - Maybe it doesn't and I misread them. Sorry. Upon rereading, it looks like a client would set TAP_CONNECT_TAP_FIX_FLAG_BYTEORDER and then in the received mutations, we'll have TAP_FLAG_NETWORK_BYTE_ORDER set. Thanks!

          People

          • Assignee:
            trond Trond Norbye
            Reporter:
            ingenthr Matt Ingenthron
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Gerrit Reviews

              There are no open Gerrit changes