Roshan Naik A couple of comments on this patch:
1. All delegators in WebHCat take the 'user' as determined by Server.java and use that to make secure calls to JobTrakcer, HDFS etc. HCatStreamingDelegator ignores it. Why is that?
2. Most operations in HCatStreamingDelegator do multiple things (like modify metadata, create some HDFS file, etc.). It sounds like every one of these operations should be atomic. For example, say for some reason 2 identical calls to partitionRoll() happen at the same time. How is this atomicity achieved?

Eugene Koifman
added a comment - 09/Sep/13 18:46 Roshan Naik A couple of comments on this patch:
1. All delegators in WebHCat take the 'user' as determined by Server.java and use that to make secure calls to JobTrakcer, HDFS etc. HCatStreamingDelegator ignores it. Why is that?
2. Most operations in HCatStreamingDelegator do multiple things (like modify metadata, create some HDFS file, etc.). It sounds like every one of these operations should be atomic. For example, say for some reason 2 identical calls to partitionRoll() happen at the same time. How is this atomicity achieved?

On Pt 1.

Thanks. I need to take a closer look at this.

On Pt 2.

I think you mean 'safe to invoke concurrently' instead of 'atomic', since the intermediate states are going to be visible when an operation spans both file system and meta store. Here is a summary of the reasons why each operation is concurrency safe:

streamingStatus : Readonly metastore operation

chunkGet : This is an atomic metastore operation

chunkAbort : Just deletes a file. So no concurrency issues here.

chunkCommit : Just renames a file. So only one of concurrent operations will succeed.

disableStreaming : This is an atomic metastore operation

enableStreaming : Does a couple of mkdirs (for setup) followed by an atomic metastore operation. mkdirs() is idempotent, so all concurrent calls succeed. All concurrent invocations enter a transaction to do the metastore update atomically...only one should update metastore.

partitionRoll : Creates empty dir for the new current partition & then atomically updates metastore as follows:

Make note of this new current partition dir

Do an addPartition() on the previous current partition.

If concurrent partitionRoll() invocations use same arguments, the addPartition() step will fail on all but one. If arguments are not same in concurrent invocations, they all succeed and updates made by the last invocation to exit the metastore transaction would override the others.

Roshan Naik
added a comment - 17/Sep/13 05:20 Thanks Eugene Koifman for the comments:
On Pt 1.
Thanks. I need to take a closer look at this.
On Pt 2.
I think you mean 'safe to invoke concurrently' instead of 'atomic', since the intermediate states are going to be visible when an operation spans both file system and meta store. Here is a summary of the reasons why each operation is concurrency safe:
streamingStatus : Readonly metastore operation
chunkGet : This is an atomic metastore operation
chunkAbort : Just deletes a file. So no concurrency issues here.
chunkCommit : Just renames a file. So only one of concurrent operations will succeed.
disableStreaming : This is an atomic metastore operation
enableStreaming : Does a couple of mkdirs (for setup) followed by an atomic metastore operation. mkdirs() is idempotent, so all concurrent calls succeed. All concurrent invocations enter a transaction to do the metastore update atomically...only one should update metastore.
partitionRoll : Creates empty dir for the new current partition & then atomically updates metastore as follows:
Make note of this new current partition dir
Do an addPartition() on the previous current partition.
If concurrent partitionRoll() invocations use same arguments, the addPartition() step will fail on all but one. If arguments are not same in concurrent invocations, they all succeed and updates made by the last invocation to exit the metastore transaction would override the others.

We should try to eliminate the need of intermediate staging area while rolling on new partitions. Seems like there should not be any gotchas while moving data from streaming dir to partition dir directly.
We should make thrift apis in metastore forward compatible. One way to do that is to use struct (which contains all parameters) instead of passing in list of arguments.
We should try to leave TBLS table untouched in backend db. That will simplify upgrade story. One way to do that is to have all new columns in a new table and than add constraints for this new table.

Roshan Naik
added a comment - 24/Sep/13 23:50 Capturing API related comments from Ashutosh Chauhan noted here in HIVE-4196
We should try to eliminate the need of intermediate staging area while rolling on new partitions. Seems like there should not be any gotchas while moving data from streaming dir to partition dir directly.
We should make thrift apis in metastore forward compatible. One way to do that is to use struct (which contains all parameters) instead of passing in list of arguments.
We should try to leave TBLS table untouched in backend db. That will simplify upgrade story. One way to do that is to have all new columns in a new table and than add constraints for this new table.

We should try to eliminate the need of intermediate staging area while rolling on new partitions. Seems like there should not be any gotchas while moving data from streaming dir to partition dir directly.

Thanks. That change is already part of the patch.

We should make thrift apis in metastore forward compatible. One way to do that is to use struct (which contains all parameters) instead of passing in list of arguments.

Roshan Naik
added a comment - 24/Sep/13 23:55 We should try to eliminate the need of intermediate staging area while rolling on new partitions. Seems like there should not be any gotchas while moving data from streaming dir to partition dir directly.
Thanks. That change is already part of the patch.
We should make thrift apis in metastore forward compatible. One way to do that is to use struct (which contains all parameters) instead of passing in list of arguments.
Hate it .. but Ok.
We should try to leave TBLS table untouched in backend db.
Sure. Will move them to a new table.