[HUDI-316] : Hbase qps repartition writestatus

[v3nkatesh]

Tips

• Thank you very much for contributing to Apache Hudi.
• Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

This pull request optimizes hbase index write operations.

Brief change log

• Replaces Thread.sleep() with RateLimiter to reduce wait time during hbase puts operation
• Repartitions WriteStatus with new records to improve parallelism of hbase index operations

Verify this pull request

This change added tests and can be verified as follows:

• Added tests to TestHbaseIndex to verify the repartition optimization
• Also verified the change by running a ob end to end

Committer checklist

• Has a corresponding JIRA in PR title & commit

• Commit message is descriptive of the change

• CI is green

• Necessary doc changes done or have another open PR

• For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

[vinothchandar]

Could we please avoid using this? Should not be too hard to roll our own..

[v3nkatesh]

@vinothchandar Re implemented for most part and stripped unnecessary methods of RateLimiter. And removed Ticker class as well (that was from Google/Guava).

[n3nash]

@satishkotha can you please help review this ?

[satishkotha]

@v3nkatesh Added some questions/comments. Please take a look.

[satishkotha]

Do you need this map as instance variable? Looks like there is one HBaseIndex object per client. We don't seem to be clearing entries from this map also. So, over time, this map can get pretty large and can cause increased memory utilization? Please correct me if I'm misreading.

[v3nkatesh]

Yeah we don't need a class variable for this. Moved it inside updateLocation() method.

[satishkotha]

I dont see how this is used. could you please add a comment for all these instance variables? It seems like they can be local variables specific to the operation being performed.

[v3nkatesh]

Yeah can be removed actually, I am anyway recalculating this inside getBatchSize(). Also cleaned up other variables that are not used.

[satishkotha]

Can you help me understand this code? why do we need to force trigger here? Is this just to releaseQPSResources? releaseQPSResources seems to be doing nothing (at least default implementation, are there other implementations outside hoodie?). Is it really important to release here as opposed to doing it in 'close()' (earlier behavior)?

[v3nkatesh]

Correct, releaseQPSResources() has implementation outside of hoodie. This ensures that we release any resources acquired for HBase operations are released right after the spark stage is done (i.e. because of forced spark action)

[satishkotha]

Not directly related to your change, so feel free to ignore this comment. But hBaseIndexQPSResourceAllocator is instance variable. why is this again passed as argument. This seems like a consistent pattern in this class. Because we are also using exact same name for local variable, it masks instance variable and can become easily error prone if the two variables evolve to mean different things.

[v3nkatesh]

Yeah I think it makes sense to refactor it here. Let me update other parts of this class where hBaseIndexQPSResourceAllocator is passed around.

[satishkotha]

Can you share the context on why we created HBaseIndexQPSResourceAllocator? Do you think calls to RateLimiter#acquire can be made inside HBaseIndexQPSResourceAllocator#acquireQPSResources to simplify?

[v3nkatesh]

Synced offline to go over use-case outside of hoodie and why we need to rate limit here.
Just to summarize, we need to rate limit here because the actual hbase operations are handled here. And HBaseIndexQPSResourceAllocator#acquireQPSResources is mostly meant to manage metadata like checking for available resources before an operation, releasing meta resources etc.

[satishkotha]

Could you please make this a static class if its not using any instance variables of outer class

[v3nkatesh]

done.

[satishkotha]

nit: looks like this is logged in the above method call too. so i think this can be removed.

[v3nkatesh]

done.

[satishkotha]

Consider redoing this logic, because if this.numWriteStatusWithInserts == 0 , we still go through the process of generating fileIdPartitionMap which is not ideal.

Also, curious, if you did any performance measurements before and after this change. It is worth highlighting in release notes if this improvement is significant

[v3nkatesh]

When this.numWriteStatusWithInserts == 0, fileIds will be empty. So fileIdPartitionMap will also be empty in this case.

[satishkotha]

nit: can we also move hTable.get(keys) inside this if? do we need to invoke hTable.get if keys is empty?

[v3nkatesh]

Invoking hTable.get on empty keys is returning an empty Result array. But anyway, I changed it to explicitly return empty array now.

[satishkotha]

lot of code in this test seems like repetition from source code. consider refactoring this part into a library to reuse in tests if needed

[v3nkatesh]

Sure, I have moved this logic inside HBaseIndex to a separate method instead of a different class, let me know. Using this util method from tests.

[satishkotha]

another question, what is the typical latency of these mutate operations? If time taken here combined with time taken to collect 'multiPutBatchSize' is > 1 second, then it seems like limiter would generate enough tokens for next run and would not wait at all.

[v3nkatesh]

Because of the synchronous nature, we will not acquire more permits until the operation is done even if it takes more than a second.

[satishkotha]

example to help clarify what i meant:

lets say, mutator.mutate() + flush +clear takes 2 seconds as minimum. limiter.acquire would never wait because it generates mutations.size() tokens every second. So we would never wait. looks like this is expected and we dont see it as a problem. So I'm fine with it. (if possible, having metrics on per operation wait time would help us debug any potential issues)

[v3nkatesh]

I think we synced offline for this, but let me try to summarize.
Token will be acquired only after previous batch is done, so it won't flood the system or over-utilize the cluster as planned. Though the side effect is hbase operation running slower than intended. Yes metrics on operation will be useful, will create follow up ticket.

[n3nash]

@v3nkatesh any updates on this ?

[satishkotha]

these two also seem like related to the operation being performed and not really need to be instance variables. If we can find a way to move them to local variables, that would make it cleaner.

[n3nash]

@v3nkatesh can you address or respond to this comment ?

[v3nkatesh]

These 2 are actually used at multiple places inside the class, so did not refactor. It's possible to move them inside methods, but it will mean re-calculating.

[satishkotha]

@n3nash @v3nkatesh added few more comments. these are in nice-to-have bucket. Since this is already running in production for quiet some time, you can merge given there is time pressure. I can also make additional changes in a later PR if needed.

[n3nash]

@v3nkatesh There are a couple of pending comments from @satishkotha. If you can finish up with those we can merge this PR with the condition that we need to add to LICENSE file because of the copied code in ratelimiter here -> https://github.com/apache/incubator-hudi/blob/master/LICENSE

[v3nkatesh]

@v3nkatesh There are a couple of pending comments from @satishkotha. If you can finish up with those we can merge this PR with the condition that we need to add to LICENSE file because of the copied code in ratelimiter here -> https://github.com/apache/incubator-hudi/blob/master/LICENSE

@n3nash I have replied to the remaining comments. For RateLimiter class, can you check if the new refactoring is good enough to skip the LICENSE part ?

[n3nash]

@v3nkatesh The rate limiter looks good to me but it's still inspired from guava. I'll let @vinothchandar comment since he felt strongly about implementing our own.

[vinothchandar]

@v3nkatesh allow me to give some context around why we are strict around guava.. Hudi as you know has to be dropped under many different services (hive/spark/presto,...) and guava as universal it is, presents a jar conflict nightmare...

On the re-using code itself, i know we have done this in few places in the past. but, this often leads to maintenance issues more often than not... we change the re-used code, people fix the original code.. and overtime we don't invest in getting upstream bug fixes etc.. So for small stuff like this, I prefer that we write it ourselves..

Is this the original code ? I would say if we are trimming down that file and using some parts verbatim, we are still reusing code.. (which I think is what we are doing)

The act of adding new things to LICENSE/NOTICE is not that straightforward, given we don't have an entry for guava yet.. We need to examine, guava's NOTICE, its dependencies etc.. I thought, even for you, just writing a small class and being done would be a better use of time?

[vinothchandar]

In any case, we need unit tests for the RateLimiter class..
Few alternatives ..
You can maintain the index outside Hudi (index classes are pluggable)
I can write the rate limiter for you and check it in..

let's hash this out together :)

[v3nkatesh]

In any case, we need unit tests for the RateLimiter class..
Few alternatives ..
You can maintain the index outside Hudi (index classes are pluggable)
I can write the rate limiter for you and check it in..

let's hash this out together :)

Thanks @vinothchandar. Reg. restrictions around guava makes sense. Would be great if you can check in a Rate limiter equivalent. Let me know if you don't have time for that in the near future.

[v3nkatesh]

Hi @vinothchandar Let me know if you will have time for the RateLimiter class. If not I will explore other options, pls let know.

[vinothchandar]

@v3nkatesh are you able to write the rate limiter class yourself? :) if not can @garyli1019 @xushiyan help?

[v3nkatesh]

@v3nkatesh are you able to write the rate limiter class yourself? :) if not can @garyli1019 @xushiyan help?

@vinothchandar Pushed a custom RateLimiter, pls take a look.

[vinothchandar]

will do. thanks ! @n3nash can take a pass as well

[n3nash]

Yes, I will do by Friday.

[vinothchandar]

@n3nash I tried to rebase this and noticed that the custom ratelimiter is still a reuse from Guava. Are we still pursuing this PR?

[v3nkatesh]

@n3nash I tried to rebase this and noticed that the custom ratelimiter is still a reuse from Guava. Are we still pursuing this PR?

Sry for the delay folks, I just came back from my break. @vinothchandar latest RateLimiter clss is not based on Guava. Let me know

[n3nash]

@v3nkatesh the build is failing, can you please fix it and then I can merge the PR

[codecov-io]

Codecov Report

Merging #1484 into master will increase coverage by 0.05%.
The diff coverage is 86.66%.

@@             Coverage Diff              @@
##             master    #1484      +/-   ##
============================================
+ Coverage     53.68%   53.74%   +0.05%     
- Complexity     2849     2858       +9     
============================================
  Files           359      360       +1     
  Lines         16565    16595      +30     
  Branches       1782     1782              
============================================
+ Hits           8893     8919      +26     
- Misses         6915     6919       +4     
  Partials        757      757              

Flag	Coverage Δ	Complexity Δ
#hudicli	38.37% <ø> (ø)	193.00 <ø> (ø)
#hudiclient	100.00% <ø> (ø)	0.00 <ø> (ø)
#hudicommon	54.81% <86.66%> (+0.11%)	1803.00 <9.00> (+9.00)
#hudihadoopmr	33.05% <ø> (ø)	181.00 <ø> (ø)
#hudispark	65.95% <ø> (ø)	304.00 <ø> (ø)
#huditimelineservice	62.29% <ø> (ø)	50.00 <ø> (ø)
#hudiutilities	70.09% <ø> (ø)	327.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
.../java/org/apache/hudi/common/util/RateLimiter.java	86.66% <86.66%> (ø)	9.00 <9.00> (?)

[n3nash]

LGTM