Fix starvation and busy waiting of StatusUpdaterBolt.java, add Constants.

[FelixEngl]

Requires #982, fixes various starvation and busy waiting problems in StatusUpdaterBolt.java. Everything else is from #982.

Signed-off-by: Felix Engl felix.engl@uni-bamberg.de

Thanks for contributing to StormCrawler, your efforts are appreciated!

Developer Certificate of Origin

By contributing to StormCrawler, you accept and agree to the following terms and conditions (the Developer Certificate of Origin) for your present and future contributions submitted to StormCrawler.
Please refer to the Developer Certificate of Origin section in CONTRIBUTING.md for details.

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Before opening a PR, please check that:

• You've squashed your commits into a single one
• You've described what the PR does or at least point to a related issue
• You've signed-ff your commits with 'git commit -s'
• The code is properly formatted with 'mvn git-code-format:format-code -Dgcf.globPattern=**/*'

Thanks!

[FelixEngl]

Why is it failing? I didn't even touch the class causing the error.

[FelixEngl]

Why is it failing? I didn't even touch the class causing the error.

The com.digitalpebble.stormcrawler.elasticsearch.bolt.IndexerBoltTest failed in the pipeline but works on my local machine and server. We may have to trigger a new build, because the associated classes of this test were not changed in any way in this PR.

08:11:32.961 [I/O dispatcher 1] WARN  c.d.s.e.b.IndexerBolt - Could not find unacked tuples for 50571c0ffec7d295bb754b4847bdf2edace07885895ca09e5d459eeddd03c6f7
08:11:32.962 [I/O dispatcher 1] INFO  c.d.s.e.b.IndexerBolt - Bulk response [1] : items 2, waitAck 0, acked 2, failed 0
Error:  Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 47.076 s <<< FAILURE! - in com.digitalpebble.stormcrawler.elasticsearch.bolt.IndexerBoltTest
Error:  simultaneousCanonicals(com.digitalpebble.stormcrawler.elasticsearch.bolt.IndexerBoltTest)  Time elapsed: 46.961 s  <<< FAILURE!
java.lang.AssertionError: expected:<2> but was:<1>
	at com.digitalpebble.stormcrawler.elasticsearch.bolt.IndexerBoltTest.simultaneousCanonicals(IndexerBoltTest.java:118)

[jnioche]

The freak unrelated error might go by itself next time round.
can you please explain what you are trying to solve with this PR? I am not sure I understand why there could be starvation here. With sections having been moved around the class, there is a large amount of diff even though the actual differences are probably not that many.

[FelixEngl]

The freak unrelated error might go by itself next time round. can you please explain what you are trying to solve with this PR? I am not sure I understand why there could be starvation here. With sections having been moved around the class, there is a large amount of diff even though the actual differences are probably not that many.

To sum up the proposed changes:

• Reordered the prepare
• Add more LOG-Events
• Replace synchronize(Object) (single read - single write) with ReentrantReadWriteLock(fair=true) (multi read - single write)
• Replace Atomic<Int> + busy waiting with Semaphore + Permits (Faster)
• Only lock the critical read/write-parts (everything else isn't blocking anymore)
• Add constants.
• Reordering the code to be more optimal with the lock-usage

Causes for replacements:

• synchronize(Object) is an unfair locking mechanism, but we are on a time-limit (cache) when processing the tuples. Therefore we need a fair lock, that makes sure, that older lock requests get processed earlier than newer ones. (ReentrantReadWriteLock(fair=true)) Otherwise there could be the case of starvation. (It is unlikely, but it can happen when one thread always looses when trying to acquire the lock on waitAck.)
• The old code is locking waitAck for basically the whole execution of store and onNext. By using the locks like this, we are loosing a lot of time by syncing code that could be run asynchronously, because everything except the read-write-accesses on the waitAck can be done in async. Furthermore we are loosing a lot of time by blocking the thread calling store when the URlFrontierGrpc-thread calls onNext, because there can be only one acquiring the lock on waitAck. To achieve an optimal throughput with minimal locking, we have to limit the locking blocks to the read-write-accesses on waitAck and utilize a ReentrantReadWriteLock where we can either perform many parallel read-accesses or a single write-access.
• The old busy waiting has a problem, that the wait time increases by every iteration and we could reach a point, where we have n-Threads that sleep for x Seconds and none of this threads can progress. By using a semaphore the delay will be as minimal as possible, because there is no real busy waiting when the semaphore handles the permit-count and the wait-releases of the waiting threads. (Furthermore we don't have to manage the permit-count by an atomic.)

p.s. if the text is too hard to read/understand please don't hesitate to ask. I'm on my phone at the moment and writing longer texts with that little thing is kind of hard. (or at least harder than I thought it would be. I'm really not a phone guy. :D)

[FelixEngl]

The ChannelManager is no more, removed it from the title.

[jnioche]

• synchronize(Object) is an unfair locking mechanism,

I did not know that, thanks!

if the text is too hard to read/understand please don't hesitate to ask. I'm on my phone at the moment and writing longer texts with that little thing is kind of hard. (or at least harder than I thought it would be. I'm really not a phone guy. :D)

this was the perfect level of explanation, thanks a lot! Can't believe you typed all that on your phone

[jnioche]

shouldn't the interrupt status be restored?

Thread.currentThread().interrupt();

[FelixEngl]

You are right, we have to handle an interrupt correctly.

I'll look up what ApacheStorm wants in this case.

Depending on that interrupt should be enough, otherwise we have to clean up manually before either throwing a new interrupt or whatever.

[FelixEngl]

Interrupt is enough, thank you!

[jnioche]

The issues you raised with the synchronization would also affect the statusupdaterbolts in other modules like the ES one. Would be good to fix it there too.
My lack of competence when it comes to multithreading is now cruelly exposed ;-)

[jnioche]

@FelixEngl do you have a Twitter account? I d like to mention your work there when it is merged

[FelixEngl]

Yes, I actually have one. (I cannot believe, that I was able to remember my login. But maybe I should use twitter a little bit more for communicating and liking projects I like :D)

https://twitter.com/EnglFelix

[FelixEngl]

The issues you raised with the synchronization would also affect the statusupdaterbolts in other modules like the ES one. Would be good to fix it there too. My lack of competence when it comes to multithreading is now cruelly exposed ;-)

When I get to it I'll look at the other modules too. But right now I have some pressure regarding other projects etc. so atm I'm just optimizing what I need for myself. It would be nice, if we could make an issue and assign me to it. That way I wont forget do do that when I have some free time that I want to spend coding. (Otherwise I swear I'll forget it if I don't get some reminders of it. 🤣)

To be honest, I am just glad that I can give something back to SC, because it is really helping me out with my phd-project. ❤️

[jnioche]

Merged, thanks @FelixEngl