You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2016/07/18 04:46:49 UTC

[GitHub] storm pull request #1574: STORM-1977 Restore logic: give up leadership when ...

GitHub user HeartSaVioR opened a pull request:

    https://github.com/apache/storm/pull/1574

    STORM-1977 Restore logic: give up leadership when elected as leader but doesn't have any of topology codes on local (1.x)

    * Nimbus H/A introduced condition for becoming a leader
    * It got removed when BlobStore was introduced
    * This patch is restoring previous logic with making logic compatible to BlobStore

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/HeartSaVioR/storm STORM-1977-1.x

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/storm/pull/1574.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1574
    
----
commit 401cd09a699d3937e0c9e800e8262fc627649da7
Author: Jungtaek Lim <ka...@gmail.com>
Date:   2016-07-18T02:51:33Z

    STORM-1977 Restore logic: give up leadership when elected as leader but doesn't have any of topology codes on local
    
    * Nimbus H/A introduced condition for becoming a leader
    * It got removed when BlobStore was introduced
    * This patch is restoring previous logic with making logic compatible to BlobStore

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/1574

@revans2
And local BlobStore should be designed to achieve hive availability just same as HDFS BlobStore. But the process BlobStore is behind is Nimbus, which is designed to fail-fast, which I think is just not same way.

For example, suppose the scenario what I addressed from STORM-1977.

Tested with the scenario I've described from STORM-1977

1. comment cleanup-corrupt-topologies! from nimbus.clj (It's a quick workaround for resolving STORM-1976), and patch Storm cluster
2. Launch Nimbus 1 (leader)
3. Run topology1
4. Kill Nimbus 1
5. Launch Nimbus 2 from different node

Without having condition for granting leadership, Nimbus 2 can grant leadership, and act as leader. This is not a blocker for BlobStore's view, since replication count for topology1 is 0 but it doesn't make them crashed, and reviving Nimbus 1 should eventually replicate topology 1 to Nimbus 2.
The thing is, leader nimbus should do the own work as Nimbus. In this case just requesting getClusterInfo can make Nimbus 2 crashed, and Nimbus 1 comes in later and gain leadership, but replication count for topology1 is still 1 until Nimbus 2 comes in.

With having condition for granting leadership, Nimbus 2 gives up leadership, and continuously waits for new leader. (No leader at that time) And Nimbus 1 comes in, and topology1 is eventually replicated to Nimbus 2 to ensure replication count.

Due to this behavior, the crash and recovery scenario heavily depends on sequence of launching Nimbuses. I think this is not a good UX.

---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    @revans2 
    Let me back to code change. Could you clarify that this change can also work well with HDFS backed BlobStore? If not how to do this?
    I'd like to address this as abstraction level of BlobStore by preventing nimbus to gain leadership when there's a topology which one of code is not available (replication count 0), but don't have an idea for how to do. If you feel more convenient to make a patch by yourself, please take over and go ahead.
    I think I should have a time to play with BlobStore by local and HDFS backed, but I'm not sure I can do that right now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by revans2 <gi...@git.apache.org>.

Github user revans2 commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    @HeartSaVioR yes there is no bug with this code.  I am +1 on merging this in as is.
    
    The bug I found is in the BlobStore implementation.  It impacts this patch in the future, because if we fix it, this will no longer work.  I just wanted to point that out.  It should not be a big deal, again because all of the nimbus instances should be synching with one another, so the race should be very small.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by revans2 <gi...@git.apache.org>.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/1574

@HeartSaVioR
I am OK with this change, like I am OK with the change for STORM-1976. I just don't think that this is the final solution for the local blobstore+nimbus, nor do I think that either is a blocker. The reality is if I use HDFS as the backing for the blobstore and I only set it to have a single replica, then I lose a datanode, nimbus will still crash in almost exactly the same way. Is this a bug in nimbus? Is it a bug in the blobstore or HDFS? I would say no. It is user error because the user is trying to make something HA without configuring it properly. Then when an error happens we cannot recover. So the question is, if it does happen is it better for nimbus to crash or is it better for nimbus to hang? Because all this does is it switches from one to the other. I can see advantages for hanging over crashing, so I am OK with this fix.

Does the blobstore code have bugs in it and things that we can change to make it work better? I would expect it to, it is software after all. I just want us to spend some time thinking about how we really want it to behave and fix it properly. If that proper fix comes after making a few patches to improve things, that is fine.

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    @revans2 
    I might be wrong with explaining my patch. My understanding is that `title` is wrong instead of `code`. No need to be in local, just need to be available at Blobstore.
    
    I didn't still see the details of Blobstore, so please correct me if I'm wrong.
    
    I had a change to review this patch myself, and realize that I didn't rely on implementation details.
    If local BlobStore is properly abstracted, BlobStore.filterAndListKeys() should handle underlying mechanism, thus it should list keys from blobstore regardless of locality of blob, and apply filter. So `code-ids` should be working well with local and HDFS, and `diff-topology` should only have missing topology code for all available blobstore replicas.
    
    If you also think title is wrong please let me know so that I can correct JIRA and PR title. Thanks in advance!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/1574

@revans2
I'm just thinking about the responsibility of Nimbus.

- Nimbus was "soft" SPOF and we claimed that Nimbus is designed to fail-fast and stateless so just supervising Nimbus works like a charm. But it doesn't help from machine failure, and moving Nimbus to other machine requires at least configuration change of whole cluster. (This assumes that Supervisor is also supervised by aux. process. If not starting Supervisor should be done manually.)
- Nimbus H/A came in. It was relatively easier than other process on other project since Nimbus is designed as stateless so no need to sync. Only thing Nimbuses should sync up is topology code, and Nimbus H/A tried to address this by full replications and restriction of becoming leader. It made some overhead trying to replicate topology codes to all of Nimbuses but it was a best try to achieve higher availability. When other nimbuses crashed but only one 'leader' nimbus was alive, that was completely OK for that moment. There was a chance for all alive nimbuses not having complete set of topology code thus no leader and hang, but it was relatively smaller than counting replication count since it was doing full replication at all.
- BlobStore came in. I don't know the details of BlobStore so hard to tell. I'd be happy if you fill out this : After BlobStore.

One thing I'm concerning is, there's new requirement for Nimbus to not easily crashed since every Nimbuses are also replica of BlobStore like DataNode, but Nimbus itself has lots of works to do (sure for leader, and not sure for followers) and it is still based on fail-fast. Is it OK to play together?

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by harshach <gi...@git.apache.org>.

Github user harshach commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    +1. Agree on having a discussion around these scenarios in another JIRA.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/1574

@revans2
I'm not clear on part of your comment,

> The one bug I saw while going through the code is that when we list keys, we are doing it only from the local storage, not form ZK. If we were doing it from ZK then when someone asks for all of the keys they would be guaranteed to get all of the keys, but this patch would do no good. Simply because it would not have an API to know what is local and what is not local. In the short term I think this is OK, but long term we need to discuss how we really want all of this to work in the different failure cases.

I think the one bug you stated is about BlobStore, right? Because `code-ids` just uses exposed BlobStore API, so underlying mechanism shouldn't be affected to its behavior. And achieving abstraction for BlobStore, like we pull a file from HDFS and expect pull will be succeed if there's at least one replica serving that file, when we use local BlobStore and pull blob which is not available for this nimbus, it should pull from another nimbus automatically without waiting for nimbus to next sync up cycle.

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by harshach <gi...@git.apache.org>.

Github user harshach commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    Thanks @HeartSaVioR . Merged into 1.0.x, 1.1-x and master.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by revans2 <gi...@git.apache.org>.

Github user revans2 commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    The code to do the leader election based off of the IDs was removed because architecturally the blobstore should be in charge of maintaining high availability.  Exposing what is stored on the local disk in an implementation detail and exposing that to nimbus breaks a lot of things.  Not to mention is not really possible with some blobstore implementations, like the HDFS backed one.
    
    To offset this we made sure that you could access the version of the blob even if it is stored on a different instance of the blobstore.
    
    Additionally the old code did not guarantee consistency either.  There was a timeout where if nimbus waited long enough to get a copy of all of the items it gave up and would still become leader.  If there really is no guarantee why add in the extra overhead?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    @revans2 
    IMO this patch will still work if we address both of the expectations: 1) BlobStore should list keys for all available keys for listing keys. 2) BlobStore should ensure that it will pull from another replica when blob is not in local. If we address only 1) it will break this patch and go back to now.
    
    Suppose both things are achieved, we would like to have nimbus giving up leadership rather than crash when we have zero replication count on one of topologies code in blobstore. Like we talked we don't want to crash Nimbus in this case, since itself is a replica of blobstore, so it incurs decreasing replication count of some part of blobs, and next leader nimbus has more chance to crash.
    
    But also as you said there should be few problem if we have enough nimbuses and syncing up blobs is enough fast and done often.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by HeartSaVioR <gi...@git.apache.org>.

Github user HeartSaVioR commented on the issue:

    https://github.com/apache/storm/pull/1574
  
    @revans2 
    If we can guarantee that leader nimbus has all topologies code, leader nimbus can function well. Old code is based on this assumption, not considering followers and also not about code distribution. It should be eventually distributed since each nimbus runs sync-code / sync-blob periodically to try to sync up. 
    
    In leader nimbus' point of view I think old code is consistent, and this code is trying to have same guarantee.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm issue #1574: STORM-1977 Restore logic: give up leadership when elected...

Posted by revans2 <gi...@git.apache.org>.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/1574

@HeartSaVioR the local file system blobstore behaves exactly the same as the nimbus HA before it. It tries to replicate all of the blobs to all of the nimbus nodes as quickly as possible. It stores the metadata for each blob in ZK, just like the original nimbus HA code. The big difference is that it can store a general set of blobs and they are all versioned so you can upload a new one, and if you ask for a blob that is not currently local on that nimbus it will speed up they sync process and go grab it from another nimbus instance for you.

There is still a race, like with nimbus HA where it may not have fully replicated, but if you set the replication count when the blob is uploaded it should be waiting for the replication to complete before declaring success.

The one bug I saw while going through the code is that when we list keys, we are doing it only from the local storage, not form ZK. If we were doing it from ZK then when someone asks for all of the keys they would be guaranteed to get all of the keys, but this patch would do no good. Simply because it would not have an API to know what is local and what is not local. In the short term I think this is OK, but long term we need to discuss how we really want all of this to work in the different failure cases.

When configured to use HDFS as the backing, all of the "blobs" are stored in HDFS. We do a directory listing to get the key listing. With that nimbus truly becomes stateless, and you can stand up a nimbus on a different node and not have to worry about it. This code would simply do the directory listing and then become a noop.