Support RowKeyFactory service protocols

[milandesai]

Part of #34. I was looking into adding a FileIdProtocol which would be used by the FileIdRowKey, and I noticed the RowKeyFactory interface still lacks support of independent RPC protocols that users can implement and configure. Assuming the user creates some RPC protocol using protobuf, we need to register the service with the coprocessor and provide the necessary RPC channel to connect to it. This means:

1. The RowKeyFactory needs to provide a com.google.protobuf.Service object that the NamespaceProcessor can register as part of its coprocessor. It can be null in the case of FullPathRowKeyFactory. This is the server-side implementation of the protocol.
2. The RowKeyFactory needs to be able to generate a RpcChannel connected to the coprocessor based on the row key. That will provide client access to the protocol. For now, I supply the Table reference to the factory. The table reference can also be used on the server side to construct an INodeManager for the protocol implementation.

Together, this leads me to introduce 2 fields in RowKeyFactory: com.google.protobuf.Service, which is set by the subclass itself, and Table. Together, those are enough to allow any external RowKey implementation to introduce a protocol. That also means we don't need the generic in RowKeyFactory. Currently, it represents the protocol interface, but Giraffa doesn't need this interface; it needs the protobuf.Service reference. Since Giraffa doesn't need to know anything about it, it makes sense to remove it. I am attaching a patch; let me know what you think.

[shvachko]

Hey Milan, looked at your patch. It would be good if you could add FileIdProtocol to your branch. So that we could have a use case for protobuf.Service fields you introduced, and see how it is going to be used. Or is it a big change?

[milandesai]

Added FileIdProtocol to the branch as an example to see how the key factory framework is used (along with some additional changes to support it). There are TODO methods in FileIdProcessor and FileIdRowKeyFactory to show what would be implemented in #34. Note how everything fits nicely into a separate "fileidrowkey" package.

It would be nice to decouple FileIdProcessor from the key factory entirely. That way we could remove all of the protobuf.Service fields from the factory. It doesn't make too much sense for the key factory to know about the service implementation anyway. One way to do this is to just have the key factory implementor create and configure their own coprocessor for the service, which is fairly straightforward. I don't know what the overhead of having multiple coprocessors is though, and if its too high, another option is to have a Giraffa configuration that specifies classes for the NamespaceProcessor to load and register.

[milandesai]

I pushed a change that implements the FileIdProcessor as a coprocessor, as discussed above. Now there are two configurations needed to install the key factory: "giraffa.rowkey.factory.class" sets to FileIdRowKeyFactory, and "hbase.region.coprocessor.classes" has FileIdProcessor.class. To see what it looks like the old way, revert the latest commit.

[shvachko]

Hey Milan, something is not adding up. Latest branch is not compiling because keyFactory does not have Service field. See NamespaceProcessor. Also there should be a way to import hdfs and Security protos from the dependencies. No? This is now definitely progress!

[milandesai]

Yes sorry, fixed that. I'll look into importing those protos from dependencies.

What do you think about the design?

[shvachko]

The design looks good, as I said good progress. Let me poke around with the patch now that you fixed the build.

[milandesai]

Reverted the FileIdRowKey implementation, as discussed with Plamen, as it was just for demonstration. Current patch is ready for review.

[shvachko]

Hey Milan, could you rebase this. It's been a while, even before gradle.

[milandesai]

Merged trunk

[milandesai]

All tests passing successfully.

[shvachko]

Hey, Milan. The problem with your approach is that you are adding hbase types into giraffa classes like RowKeyFactory, which is not right. HBase types should stay in hbase packages rather than spreading all around. I believe that was the purpose of the generic parameter <S>. What do you think?

[milandesai]

Hey Konstantin, the problem with using the generic parameter is that I think it's a misuse of generics that could lead to other problems in the future. If a RowKeyFactory implementation has a generic parameter specifying class S, everyone that calls RowKeyFactoryProvider#newInstance (such as NamespaceProcessor and NamespaceAgent) must provide an implementation of S as an argument. However, the generic hides away this required type and makes it seem as if there are other acceptable types. The truth is that the RowKeyFactoryProvider is not "generic" at all, and if we provide an incorrect type (e.g. any class other than S), there will be a ClassCastException at runtime rather than a compile time error.

In this case, FileIdRowKeyFactory (or any other RowKeyFactory implementation connecting to a coprocessor) needs some way to get an RPC channel to its coprocessor. In the current patch, this is done by supplying the hbase.client.Table interface as an argument, with the added side effect of producing an hbase dependency in a few classes. We can solve this by keeping the generics and make FileIdRowKeyFactory specify a Table as its generic parameter, but that runs into the issue described above. I think a better option is to still remove the generics as in my current patch, but replace Table with our own interface. This interface will have a single method that takes a byte[] key and returns a com.google.protobuf.RpcChannel. NamespaceProcessor and other classes that need RowKeys will supply an HBase-specific implementation of this interface, but this way the HBase references are kept completely out of the RowKey logic. I'll post an updated patch tomorrow but let me know what you think.

[milandesai]

Pushed a new commit that removes the HBase dependencies from RowKeyFactory and RowKeyFactoryProvider. The difference between trunk and this pull request is now very simple: the generic parameter for RowKeyFactory has been replaced with a concrete argument of type RpcService. The RpcService interface exposes a single method, #getRpcChannel, which takes a byte[] key and returns a BlockingRpcChannel connecting to the remote protocol. NamespaceProcessor and other classes constructing a RowKey supply an HBase-specific implementation of RpcService called HBaseRpcService which assumes the remote protocol is a coprocessor.

[milandesai]

Another possibility is that the row key implementation creates its own Table connection instead of being given it by the caller. As long as it has access to the Configuration, it can grab the Giraffa table name and open a connection to the coprocessor all on its own. To show this implementation, I pushed two new commits. The first removes the RpcService argument I added and instead allows a RowKeyFactory to be initialized with a Configuration object. It also added some minor changes to support the following commit. The second introduces the FileIdRowKey implementation that creates its own Coprocessor connection based just on the provided Configuration. The hbase-specific logic is restricted to the FileIdAgent class in the hbase package so that FileIdRowKeyFactory remains free of HBase dependencies. This could work because we generally only initialize the RowKeyFactory once, so the overhead of instantiating a new coprocessor connection is restricted to the initialization. Furthermore, HBase might be caching the Connections internally, though I'm not sure. Please take a look at the new changes. The second commit is for demonstration and can be done in #34 instead.

[shvachko]

Looked at your latest patch, Milan. Just to clarify. I am not against removing the generic parameter. But I don't want any hbase dependencies in the API for FileIdRowKey. This should be generic enough to work with any implementation of distributed namespace service. Right now it is HBase, but we should be able to use substitute it with other DBs or KV stores. This is the main requirement for me. With your last approach you hid hbase dependencies one step deeper in FileIdAgent, but it is still referenced in FileIdRowKey. I cannot replace HBase. There are ways to define API that way without generics. One is to introduce an interface that is used in FileIdRowKey with an actual implementation in hbase package. I am even fine if FileIdRowKey itself is abstract with some fields defined in hbase.FileIdRowKeyImpl. Or FileIdAgent is an interface with hbase.FileIdAgentImpl. A simple criteria is that there is no word "hbase" anywhere in FileIdRowKey.java Does it make sense? LMK if you want me to refactor your code as a demo.

[milandesai]

Thanks Konstantin, I see what you mean. You mentioned that one possibility is using an interface in lieu of FileIdAgent. That is a good point, as FileIdAgent is already an implementation of the interface FileIdProtocol, which can be moved out of the hbase package as it has no hbase dependencies. So we could replace FileIdAgent with FileIdProtocol in FileIdRowKey.

One thing though, where should we construct the actual FileIdAgent? Would it be okay to construct it in FileIdRowKeyFactory, or should that also be free of *.giraffa.hbase dependencies?

[shvachko]

FileIdRowKeyFactory can be HBase specific. As long as we deal only with RowKeyFactory interface inside giraffa packages.

[milandesai]

Sounds good. Updated the code to ensure there's no hbase dependency in FileIdRowKey. Note that I don't intend on committing the FileIdRowKey stuff in this issue; it will eventually be reverted from this PR. It's just here for demonstration purposes and there's more changes needed to actually get id-based row keys to work. So as far as what would be committed in this issue, nothing has changed.

[shvachko]

Unless I missed something, but I still see import org.apache.giraffa.hbase.fileid.FileIdAgent; in FileIdRowKeyFactory. Looks like a dependency on HBase to me. Also it gives me some compile errors in GiraffaTestUtils.

[milandesai]

Konst, I must have misunderstood you. I thought you said earlier that FileIdRowKeyFactory can be hbase specific?

Fixed the compilation errors in test, the gradle commands I was running didn't catch them for some reason.

[shvachko]

Yes it can, but then it should be in hbase package itself.

[milandesai]

Changed it so that FileIdRowKeyFactory references the hbase-independent FileIdProtocol as well. FileIdAgent is reflectively through the configuration. The code is now analogous to the way we deal with NamespaceAgent (GiraffaClient references NamespaceService and loads the implementation from GiraffaConfiguration)

[shvachko]

That looks like the right approach. I remember we compromised on leaving hbase dependencies in Configuration class for defining default class implementations.

1. I see tests are failing.
2. Also let's correct license comments. FileIdRowKey doesn't have one, in other new classes ASF license should be JavaDoc, not a regular comment.

[milandesai]

I wasn't planning on committing the FileIdRowKey implementation here - just the structural changes that would permit it. I plan on committing the actual implementation in #34 and posted a partial implementation here just for demonstration purposes. There's still work that needs to be done on that front, hence the test failures, but it will be done in #34.

I've reverted the FileIdRowKey related changes from this PR. Tests are now passing.

[shvachko]

Could you please restore ASF license as JavaDoc comment in RowKeyFactory. Otherwise +1, lets commit it.

[milandesai]

Fixed and pushed to trunk as 8554de7d9874f4a2616fe1ca5f38bab386828194. Thanks Konst!