Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make bookie automatically create folders on new machine. #567

Closed
ivankelly opened this issue Oct 17, 2017 · 1 comment
Closed

Make bookie automatically create folders on new machine. #567

ivankelly opened this issue Oct 17, 2017 · 1 comment

Comments

@ivankelly
Copy link
Contributor

JIRA: https://issues.apache.org/jira/browse/BOOKKEEPER-1099

Reporter: Zhimeng Shi

When bookie start and found it need run bookie format to create folders it should do that automatically. That way will make new machine deployment and machine replacement process much simpler without manually call the bookieformat command.

Comments from JIRA


Sijie Guo 2017-06-21T23:52:05.782+0000

[~zshi] are you planning to change the behavior in the code or the script?


Enrico Olivelli 2017-06-22T06:40:20.831+0000

I have always run Bookies in "embedded" mode, that is to run the bookie inside a JVM launched not but the standard shell script, and my usual start up procedure include a call to the Bookie format function.
It is already smart enough to leave the disk "as it is" if the layout is correct.

I think it would be good to add a bookie configuration option to run
{code:java}
BookKeeperAdmin.format(config, false, false)
{code}

This change is small, and I think it will simply the "docker" image too

If my proposal is OK I can file a PR or a BP and include it in 4.5.0


Sijie Guo 2017-06-22T06:44:55.802+0000

My concern is putting any format code on startup is dangerous. BookKeeper should just provide the admin utility and leave this to deployment scripts. The deployment script should handle things like bookie addition, retire. It should not be the code part of starting procedure.


Sijie Guo 2017-06-22T06:48:45.325+0000

[~eolivelli] for smaller changes (not big features), let's use JIRA, Github Issues or PR. BP should only be used for big features/changes, public API changes and such. For example, security is a big feature, which it can be a BP, but adding an auth plugin can just simply be an issue; adding a new interface for reading uncommitted entries is a public API change, which it can be a BP. I don't suggest to overuse BP.


Enrico Olivelli 2017-06-22T06:50:53.343+0000

[~hustlmsp] I see your point.
But in my (limited) experience of running Bookies I never got into troubles.
I must say that I always run very simple configurations, i.e. with one single disk per bookie.

I understand that is is dangerous and I am fine it the community wants to close this issue as 'won't do'.


Flavio Junqueira 2017-06-22T13:56:33.398+0000

I agree with [~hustlmsp]. If you know what you're doing and want to embed the formatting, then it's fine, but having it done automatically and by default could lead to undesirable behavior and it touches the bookkeeper metadata.


Enrico Olivelli 2017-06-22T14:12:18.246+0000

Maybe we could add a configuration option and make it default to "disabled"


Sijie Guo 2017-06-22T15:50:45.047+0000

How to reliably check if you need bookieformat is tricky. Any misconfiguration or bug can easily destroy the data. Also it is so hard to manage such configuration in large scale.

Formatting a bookie typically happen on new bookies, only when you do cluster expansions. This is part of bookie lifecycle management and typically tight to the deployment procedure.


Enrico Olivelli 2017-06-22T15:54:54.799+0000

I am fine with closing this issue as won't do


Sijie Guo 2017-06-22T17:57:33.345+0000

[~eolivelli] we need inputs from Zhimeng before closing the issue.


Zhimeng Shi 2017-06-22T18:50:39.146+0000

Thanks for Sijie's explanation in email. I guess I should reply here rather than in email directly.

When I start working on Salesforce as first time touch open source world, I really don't understand why we use a so compplicated Puppet system to setup machines. why not move all the service related setup logic into the deployment step etc. I think Sijjie's explanation gave me some sight about the reasons.

Anyway, for this issue, even we want be flexible on the working disk, the machine prepare setup like Puppet or other system should only create the root working folder (in Salesforce that folder is /xxx/sfsdata). Then let the service self create sub folders (journal folder and ledger folders here) needed under it. Because that way can decouple the service internal file structure and logic from the machine/disk setup process. If in future Bookkeeper want to fully change the folder structure, you don't need modify the Puppet model for the machine disk prepare.

Is that make sense? Or is it true in Twitter that you use Puppet to create all the sub folders?


Sijie Guo 2017-06-22T18:56:25.070+0000

I am not sure how does creating root working folder help here.

For example, if I have 4 disks, /dev/disk0, /dev/disk1, /dev/disk2 and /dev/disk3. I want to use /dev/disk0 as the journal directory '/data/bookkeeper/journal' and use /dev/disk{1-3} as the ledger directories '/data/bookkeeper/ledger-{1-3}'. How does your idea work out here?


Zhimeng Shi 2017-06-22T19:03:18.359+0000

In that case the Puppet setup should create /data/bookkeeper/ folder on the different disks and mount them. Then bookkeeper server self create the journal and ledger folders based on configuration.


Sijie Guo 2017-06-22T19:21:18.654+0000

how do you mount them? you need to create /data/bookkeeper/journal first in order to mount /dev/disk0 to this directory.


Zhimeng Shi 2017-06-22T19:31:37.179+0000

ok I didn't understand the scenarios clearly. Actually we have similar setup in future to use separate disks for journal and ledger. For that case we should mount disk on different folder like /data/bookkeeper1 and /data/bookkpper2. or even /data1/bookkeeper and /data2/bookkeeper, which is more useful when run multiple services on same machine, because other service may also want store different data on different disks.

Basically my point is still decouple the disk setup from the internal folders of the service self. And make deployment simpler.


Sijie Guo 2017-06-22T22:15:51.464+0000

[~zshi] I agreed with decoupling the disk setup from the internal folders.

what are the internal folders that you have in your mind? for example, if I configure the directories as below:

{quote}
journalDirectories=/data/bookkeeper/journal-0,/data/bookkeeper/journal-1
ledgerDirectories=/data/bookkeeper/ledger-0,/data/bookkeeper/ledger-1
{quote}

What are the directories that you propose to create during the startup?


Zhimeng Shi 2017-06-23T00:07:52.125+0000

I think it need create the journal-0, journal-1 and ledger-0 and ledger-1. because it can read these names from config file, and create them based on that.


Sijie Guo 2017-06-23T00:49:51.333+0000

[~zshi] if so, that's the point I am concerned and trying to explain to you.

because we want 4 different disks are mounted to them. for example, /dev/disk0 mount to /data/bookkeeper/journal-0, /dev/disk1 to /data/bookkeeper/journal-1, /dev/disk2 to /data/bookkeeper/ledger-0 and /dev/disk3 to /data/bookkeeper/ledger-1. you can't do this in the code, you have to do this in your deployment script, which it is tight with your deployment procedure.


Zhimeng Shi 2017-06-23T18:14:28.744+0000

I think the problem now is how do we mount the disks. in Salesforce we will mount them on upper level folder like the /data1/bookkeeper and /data2/bookkeeper. In Twitter you will mount them directly on the final folder, I think that is fine for different preference. So I have another proposal may resolve this problem better:

  1. Add a new function like CheckAndInitFolders(), which will only create the folders when found they are not exist. This way can avoid the risk to loss data like calling format automatically.
  2. Make this function call configurable in config file. So in Salesforce we will enable it, and in Twitter you can disable it. Actually even enable it will not do harm because it will first check if the folder already exist, which is true in your case.

Is that good for you?


Sijie Guo 2017-06-23T21:00:39.860+0000

[~zshi] If you already have the script to create the directory and mount disks, why don't you just create the sub-directories along with this process?

Or I can ask in a different way: you are saying creating /data1/bookkeeper and mount the disk, and creating /data1/bookkeeper/journal and /data1/bookkeeper/ledgers. That means /data1/bookkeeper/journal and /data1/bookkeeper/ledgers are on the same disk. Why can't you just configure journalDirectory=/data1/bookkeeper and ledgerDirectories=/data1/bookkeeper? Isn't that simpler? Because I don't see any point to have separate directory if they are on same disk.

I am also not in favor of having a flag like this. It increases a lot of unknowns. We should just keep a simple contract between bookkeeper process and deployment scripts. Deployment scripts should handle creating the directories and mounting the disk layouts, the bookkeeper process should just take the directories to work on, if the directories are not created, that means the machine is not well prepared, it should fail.


Zhimeng Shi 2017-06-23T21:18:40.168+0000

The problem is we don't have the script to create the directory. Each time deploy to new machine we need manually call the bookie format command to create the folders (also in one box test). Although I want to make it work on the deployment script, it need read the configure file to know the folder name, and we also start support different config value for different cluster, which need more logic to parse the config file. That's why I want to add this logic into the code rather than script to parse the config again.

I think there is nothing wrong with either way, it's just preference. I like to make thing simpler, which can also benefit one box case and container case. And adding a InitFolder function will not hurt anything, but I can understand your preferred way to mount disk too.

Software and programming is so flexible, so in this case I will respect owner's decision. I'll discuss with JV with the final solution.


Sijie Guo 2017-06-23T22:05:43.584+0000

[~zshi] just to clarify what I said: I am fine with providing any tools in BookKeeperAdmin or BookieShell that simplifies your logic. My concern is not putting such logic into bookie startup code. The bookie startup code should just hold a simple deal: Give me a few directories to start, I start up only when I find the directories and the directories have the proper cookies (to prevent any mistakes happening). If there is anything wrong (directories are missing, cookies are missing), I should not start.

There are some operations should only execute once - for example, 'metaformat' should only happen once when setting up the cluster, 'bookieformat' should only happen once when adding a bookie to a cluster. Putting such logic in the code and execute everytime the bookie process is restarted, is dangerous. 'InitFold' is a similar thing - it might not be a big deal as 'metaformat' and 'bookieformat'. But it is still concerned since it might potentially misconfigured the disk layout in the way that you are not expecting.

It is not my decision. It is a community project, if there are other people feeling this is okay, I am fine with that. so it is a -0 from me.


Zhimeng Shi 2017-06-23T22:36:45.462+0000

ok, I can understand that. Then I will add a initFolder function, but will not call it directly in Bookie, but call it by bookieShell. Then I'll add that bookieshell call into our start up script. Although it's a little bit complicated, but should works fine. the reason I want it in java code still because I don't want write another script logic to parse the config file. Is this good?

@ivankelly
Copy link
Contributor Author

Closing this as it's not a feature we will implement.

athanatos pushed a commit to athanatos/bookkeeper that referenced this issue Jan 25, 2019
Author: Suyog Mapara <suyogm@fb.com>
Author: Suyog Mapara <suyogm@devvm30625.prn1.facebook.com>

Reviewers: breed@apache.org, andor@apache.org

Closes apache#567 from suyogmapara/ZOOKEEPER-3071 and squashes the following commits:

0a2899994 [Suyog Mapara] Addressing comments
9d5765181 [Suyog Mapara] Update documentation per comments.
5201329bc [Suyog Mapara] Updating documentation
6d2b72058 [Suyog Mapara] Addressing comments
9386e2b6c [Suyog Mapara] ZOOKEEPER-3071: Updating comment to include more details about the feature
5a28efd72 [Suyog Mapara] ZOOKEEPER-3071: Addressing comments and fixing incorrect merge.
1aa12f9d3 [Suyog Mapara] ZOOKEEPER-3071: Add a config parameter to control transaction log size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant