Accumulo
  1. Accumulo
  2. ACCUMULO-756

Thrift API should be removed and abstracted

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: build, rpc
    • Labels:

      Description

      The thrift API for communication between components should be abstracted out and made more generic, so we can plug in different communications mechanisms. The thrift code generation and the generated code should be moved to a specific implementation of this abstraction, and put in a separate sub-module.

      This can help with:
      1. Making full mocking support easier (ACCUMULO-14) with an in-process "RPC" implementation.
      2. Testing alternative protocols, like Avro, Protocol Buffers, SSL, etc.
      3. Minimizing dependencies and isolating thrift generated code from code that is maintained (ACCUMULO-493).
      4. Wire compatibility (ACCUMULO-751).
      5. More?

      1. ACCUMULO-756.patch
        6.81 MB
        Christopher Tubbs

        Issue Links

          Activity

          Hide
          Christopher Tubbs added a comment -

          I quickly modified the POM files and moved the generated code around to put the thrift stuff in a separate module to see how easy this would be. There are some issues:

          1. I don't have commit access yet, so I can't "svn mv" to keep svn history during refactoring (so I'll need to work with a committer to reproduce those in a sequence of commits, or else we lose that info in the patch I would otherwise provide).

          2. I don't have a sense of what the abstraction would look like, except possibly just looking at the thrift definitions and creating Java interfaces that match. Without that, I can simply move the thrift stuff aside and update the dependencies in the build (which would still be useful on its own), so we'll need some work on creating the interfaces for the abstraction.

          3. We need a name for the sub-modules. I was originally going to call it "accumulo-api", with "accumulo-api-thrift" as the artifactId for the abstraction and thrift implementation, respectively. However, "api" is misleading, because somebody might think this contains the public API, which is actually in the "accumulo-core" artifact (should probably be in a "accumulo-client" one, but that's a separate concern). "accumulo-rpc" was suggested (with "accumulo-rpc-thrift" being an implementation).

          4. This is not useful to consider yet, because there is only one implementation, but at some point, some consideration is going to need to be made to decide how to swap out implementations. This could be done with an annotation scanning framework with drop-in jars, a configuration change with classes being available on the classpath, or a build-time configuration. This is a future consideration, though. Not relevant for this ticket... just documenting to put it on record.

          Show
          Christopher Tubbs added a comment - I quickly modified the POM files and moved the generated code around to put the thrift stuff in a separate module to see how easy this would be. There are some issues: 1. I don't have commit access yet, so I can't "svn mv" to keep svn history during refactoring (so I'll need to work with a committer to reproduce those in a sequence of commits, or else we lose that info in the patch I would otherwise provide). 2. I don't have a sense of what the abstraction would look like, except possibly just looking at the thrift definitions and creating Java interfaces that match. Without that, I can simply move the thrift stuff aside and update the dependencies in the build (which would still be useful on its own), so we'll need some work on creating the interfaces for the abstraction. 3. We need a name for the sub-modules. I was originally going to call it "accumulo-api", with "accumulo-api-thrift" as the artifactId for the abstraction and thrift implementation, respectively. However, "api" is misleading, because somebody might think this contains the public API, which is actually in the "accumulo-core" artifact (should probably be in a "accumulo-client" one, but that's a separate concern). "accumulo-rpc" was suggested (with "accumulo-rpc-thrift" being an implementation). 4. This is not useful to consider yet, because there is only one implementation, but at some point, some consideration is going to need to be made to decide how to swap out implementations. This could be done with an annotation scanning framework with drop-in jars, a configuration change with classes being available on the classpath, or a build-time configuration. This is a future consideration, though. Not relevant for this ticket... just documenting to put it on record.
          Hide
          Christopher Tubbs added a comment -

          This is a first stab... it's not as big as it looks. Much of the change is due to the svn mv of the generated code. ACCUMULO-756.patch

          Show
          Christopher Tubbs added a comment - This is a first stab... it's not as big as it looks. Much of the change is due to the svn mv of the generated code. ACCUMULO-756.patch
          Hide
          Keith Turner added a comment -

          One problem with this ZooKeeperInstance.getConnector(AuthInfo auth). This is a public method thats part of the public API, your patch changed the package of AuthInfo so it will break API compat. I do not know why AuthInfo is in the public API, it should not be, but it is. The fact that it should not be there is one issue, but it is there and users are probably using it so it should not change.

          I create a little script to diff the API. You can run this, its in test/compat/diffAPI.pl. It will generate a diff of the public API. I just ran it to compate 1.4 w/ trunk and saw some issues unrelated to your change. I did not apply your patch, I just took a glance at it. I was aware of the issue with AuthInfo being in the public API and caught that. AuthInfo being in the public API sorta annoys me because its a thrift object, I hope I am not the one who did that

          Show
          Keith Turner added a comment - One problem with this ZooKeeperInstance.getConnector(AuthInfo auth). This is a public method thats part of the public API, your patch changed the package of AuthInfo so it will break API compat. I do not know why AuthInfo is in the public API, it should not be, but it is. The fact that it should not be there is one issue, but it is there and users are probably using it so it should not change. I create a little script to diff the API. You can run this, its in test/compat/diffAPI.pl. It will generate a diff of the public API. I just ran it to compate 1.4 w/ trunk and saw some issues unrelated to your change. I did not apply your patch, I just took a glance at it. I was aware of the issue with AuthInfo being in the public API and caught that. AuthInfo being in the public API sorta annoys me because its a thrift object, I hope I am not the one who did that
          Hide
          Christopher Tubbs added a comment -

          I'm going to pick this back up after the switch to git. It'll be much easier to handle the refactoring when I can track file renames.

          Show
          Christopher Tubbs added a comment - I'm going to pick this back up after the switch to git. It'll be much easier to handle the refactoring when I can track file renames.
          Hide
          Sean Busbey added a comment -

          I'd like to benchmark changing out our Thrift RPC for Avro, but I'd much prefer to do taht on top of this ticket.

          Show
          Sean Busbey added a comment - I'd like to benchmark changing out our Thrift RPC for Avro, but I'd much prefer to do taht on top of this ticket.

            People

            • Assignee:
              Christopher Tubbs
              Reporter:
              Christopher Tubbs
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development