You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by reddycharan <gi...@git.apache.org> on 2017/06/13 06:59:30 UTC
[GitHub] bookkeeper pull request #189: BOOKKEEPER-1033: Handle DirsPartitionDuplicati...
GitHub user reddycharan opened a pull request:
https://github.com/apache/bookkeeper/pull/189
BOOKKEEPER-1033: Handle DirsPartitionDuplication
- Provide configuration for allowDirsPartitionDuplication
- while calculating total disk metrics account Partition Duplication
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/reddycharan/bookkeeper dirspartitionduplication
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/bookkeeper/pull/189.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 #189
----
commit e4c165d43e1da42214aa23cc4c652774aa1f1e68
Author: cguttapalem <cg...@salesforce.com>
Date: 2017-04-11T19:37:16Z
BOOKKEEPER-1033: Handle DirsPartitionDuplication
- Provide configuration for allowDirsPartitionDuplication
- while calculating total disk metrics account Partition Duplication
----
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by jvrao <gi...@git.apache.org>.
Github user jvrao commented on the issue:
https://github.com/apache/bookkeeper/pull/189
Multiple entrylog change is coming in and Charan is using this for that. So
you can think of this as setting stage for that.
If you don't want to have the flag/configuration parameter we can get rid
of it; That is not really needed for what we need.
If you feel strongly against this configuration param, we can flag a
warning instead of error if user configured multiple entrylogs on same
disk. Its a bit of code change and another commit not a biggie.
On Wed, Jun 28, 2017 at 9:25 AM, Sijie Guo <no...@github.com> wrote:
> re 1.
>
> I am not aware if they are users configuring in this way. but I am not
> sure if this is the case for flagging as error. because in current
> implementation, such configuration doesn't have any performance impacts. if
> this flag is necessarily for multiple entry log files, I would defer adding
> this flag when reviewing the multiple-active-entry-log-files change.
> because it is not very clear about the problem and benefits.
>
> re 2>
>
> this sounds like a policy problem - how to choose the directory to place
> the entry log files to achieve balance between disk partitions? currently
> bookie asks the ledgerdirs manager for a ledger directory. the ledger disks
> can do a better job for balancing the placement of entry log files.
>
> re 3.
>
> if we have a policy abstraction in LedgerDirsManager, we can have a better
> policy on how to pick up the entry log files for write. in this way, we can
> balance the disk usage and I/O usage.
>
> I am seeing a trend of adding more and more configuration settings into
> the bookies. some are strongly required. some doesn't seem to be really
> needed. In this pull request, the configuration doesn't seem to be the
> right solution that address the problems that you guys are listing. it
> seems to be me a flag to mask the underneath problems and this flag will
> most likely be used anymore when we have the correct solution in place.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/bookkeeper/pull/189#issuecomment-311713578>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAChrh-lox4aBr31It0IhIJUtinhtyH_ks5sIn6WgaJpZM4N4EFC>
> .
>
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@jvrao @reddycharan - that's one of my comments as well. because I don't have a clear picture what directories are concerned.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@sijie do you mean to say that remove the config and make it mandatory? I dont think we can do that since it breaks backward compatibility and for development and testing purpose there should be way to configure multiple ledgers even on machines which have single drive.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
Yes @eolivelli.
I want the option to be available for users to choose whatever layout they want and provide backward compatibility, but at the same time provide a config to restrict the users not to inadvertently configure multiple ledgerdirs in the same partition if it is not desired. To satisfy all needs this seems to be easier.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
The concern I have is - people don't have to care about this flag.
User should still configure the directories in the way how they are configuring right now. The bookie can be smart to figure about the directory and disk-partition layout. You can build the Map<FileStore, List<File>> in LedgerDirManager. So the LedgerDirManager does the de-duplication and handle disk filled up correctly. When a DiskPartition fills up, all the directories on this disk partition should be marked as non-writable.
If the concern is about the duplicated disk partitions fill the ledger directory checking. we should just fix the behavior, rather than exposing another configuration setting to the users.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
and yes, I didn't add check for journaldirectories. Will add check for journaldirectories also. Thanks for pointing that out.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
1) Because of this we would get accurate disk usage/availability metrics if multiple ledgerdirs are configured in the same drive. This is important for DiskWeightBasedPlacementPolicy.
2) Also, internally we would like to not to let users to inadvertently configure multiple ledgerdirs in the same drive , since having multiple ledgerdirs in the same drive is not going to improve performance.
This issues was initially raised by @revans2 in one of my earlier code review comments.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@sijie @eolivelli anymore comments/concerns?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@sijie considering we have come to an understanding regarding the need and purpose of this change, I'm keeping the logic as it is and just going to change the config variable name as we discussed earlier (from 'partition' to 'diskPartition') and resend the commit.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@jvrao
sorry, what is the performance problem behind this? is there anything I missed in this conversation?
If a user configures multiple journal directories, can we dedup and use one journal directory per disk partition? If we dedup, there is no performance degradation and there is no backward compatibility issue, right?
For example, we open a entrylog file by calling **ledgerDirsManager.getWritableLedgerDirsForNewLog()** , the dirs manager can handle the duplication (make sure one directory per disk partition is used) and return the right directory. By doing this, all the directory/disk partition related complexity is hidden and managed and people don't have the know the details.
I don't have any strong opinion adding another setting. My feeling is that we are not taking the right approach to address the problem here.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by reddycharan <gi...@git.apache.org>.
Github user reddycharan commented on the issue:
https://github.com/apache/bookkeeper/pull/189
with this change they introduced multiplejournals/journaldirectories - https://github.com/apache/bookkeeper/commit/123eccd435a4a96a9147ed4a24efbe9025fe79ba
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by jvrao <gi...@git.apache.org>.
Github user jvrao commented on the issue:
https://github.com/apache/bookkeeper/pull/189
In my comments I called journal dirs. but infact they are ledger dirs. Typos from my side.
As far as journals are concerned, we can have only one journal dir right? @reddycharan what do you mean by " Will add check for journaldirectories " ?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@sijie as far as I can see the flag only adds an additional check on the configuration and it does not affect the way we are going to calculate disk space (which now it will be correct).
@reddycharan Do I understand correctly ?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@reddycharan I can understand this code change but I don't see a strong reason about this change. What is the issue if you configure multiple same directories? Also the option doesn't seem to be necessary here.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@reddycharan gotcha. If that's the case, is the configuration flag really needed here?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@jvrao yes, that's the idea.
I am +0 on this. It is fine for me to adding an option, although I still don't think it is the right approach to do this.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by jvrao <gi...@git.apache.org>.
Github user jvrao commented on the issue:
https://github.com/apache/bookkeeper/pull/189
The flag is to allow backward compatibility and warn the user if they configured multiple journal dirs on the same partition by mistake. Per @sijie's point, yes we can handle them by monitoring the disk partition, and mark all dirs on that dir as non-writable. But this doesn't address the perf problem @reddycharan mentioned. I think we need to look at this in two folds.
1. Add correct metrics for reporting capacity, and keep existing behavior. This is what @reddycharan did.
2. @sijie suggesting enhancing existing behavior, which can be handeled with another Jira item.
Thoughts?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@reddycharan no, I mean - why does people care about this layout? Shall the correct fix be - the disk checker figure out which physical disk that the bookie is using and handle that correctly, rather than leaving this to configure by people.
for example, if /bookie/ledger1 /bookie/ledger2 /bookie/ledger3 are all on same disk /dev/hd1. then disk check should monitor /dev/hd1, people doesn't have to care about that, right?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
>> re 1.
I am not aware if they are users configuring in this way. but I am not sure if this is the case for flagging as error. because in current implementation, such configuration doesn't have any performance impacts. if this flag is necessarily for multiple entry log files, I would defer adding this flag when reviewing the multiple-active-entry-log-files change. because it is not very clear about the problem and benefits.
>> re 2>
this sounds like a policy problem - how to choose the directory to place the entry log files to achieve balance between disk partitions? currently bookie asks the ledgerdirs manager for a ledger directory. the ledger disks can do a better job for balancing the placement of entry log files.
>> re 3.
if we have a policy abstraction in LedgerDirsManager, we can have a better policy on how to pick up the entry log files for write. in this way, we can balance the disk usage and I/O usage.
I am seeing a trend of adding more and more configuration settings into the bookies. some are strongly required. some doesn't seem to be really needed. In this pull request, the configuration doesn't seem to be the right solution that address the problems that you guys are listing. it seems to be me a flag to mask the underneath problems and this flag will most likely be used anymore when we have the correct solution in place.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@jvrao @reddycharan
one more question, most of the comments are around the performance on journal directories. but it turns out this change has nothing to do with the journal directories. The change applies only on the ledger directories and index directories. if that's the case, I don't see a reason to have this configuration. because random I/O anyway exists on ledger and index directories, there isn't any performance difference between configuring one directory per disk partition or multiple directories per disk partition. Filesystem I/O scheduling will handle either case in the same way. so this doesn't sound like an operation error as what @jvrao said.
so back to the question, what is the problem that this pull request want to address?
- if the problem is about the mis-reported disk metrics, this can be done smarter by de-duplicating the directories within ledger disks and report the per disk partition metrics.
- if the problem is about performance, multiple ledger/index disks on same partition doesn't have any performance difference with one ledger/index disk per disk partition. this doesn't sound like an operation error that needs to be fixed.
- if the problem is about performance on journal disks, I am fine with such setting. but this pull request need to improve to address the right problem.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by jvrao <gi...@git.apache.org>.
Github user jvrao commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@sijie @reddycharan Let me step back and discuss this.
1. Are there any legitimate use cases of specifying multiple ledger dirs on the same disk partition? if not, in production I would like to flag this as error, or at least a warning to ignore other ledger dirs. I don't think code need to work hard to dedup and skip these things. This is not a naive user mistake, configurations are done by system administrator and they need to be crisp IMO.
2. Currently we have only one active entrylog, so having multiple dirs on the same disk, at the most may fill one disk partition faster/quicker than other. (Say three ledger dirs on disk1 and one ledger dir on disk2). If disk1 and disk2 are of same sizes, disk1 gets used more. but if disk1 is bigger than disk2 then this feature may be handy.
3. When we move to multiple active entrylogs, we are going to distribute entrylogs across ledger dirs. When this feature is introduced it surely hits perf because one disk may be having multiple entrylogs which gets written/scyned unless dedup is implemented.
@sijie I am not sure what is the biggest concern about this. Are you worried about too many configuration parameters? or something else?
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/bookkeeper/pull/189
@jvrao I am fine with having a configuration setting as what I said before.
The comments are mostly for people knowing different aspects on the solutions.
---
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] bookkeeper issue #189: BOOKKEEPER-1033: Handle DirsPartitionDuplication
Posted by jvrao <gi...@git.apache.org>.
Github user jvrao commented on the issue:
https://github.com/apache/bookkeeper/pull/189
Right. So if there are two dirs /journal1 and /journal2 both were created
on the same disk.
With the dedup, we will choose only one say, /journal1 and /journal2 is
always kept empty. Am I getting it right?
We do the dedup logic even while getting the disk capacity etc etc. So here
we are masking/managing mis=configuration
But our point is, configuring two directories under same disk partition is
itself wrong, unless user wanted to do it explicitly
for some test reasons. That is why we have that configuration variable,
that forces operator error, unless it is set explicitly " I know what I am
doing" kind.
Makes sense?
On Tue, Jun 27, 2017 at 2:10 PM, Sijie Guo <no...@github.com> wrote:
> @jvrao <https://github.com/jvrao>
>
> sorry, what is the performance problem behind this? is there anything I
> missed in this conversation?
>
> If a user configures multiple journal directories, can we dedup and use
> one journal directory per disk partition? If we dedup, there is no
> performance degradation and there is no backward compatibility issue, right?
>
> For example, we open a entrylog file by calling
> *ledgerDirsManager.getWritableLedgerDirsForNewLog()* , the dirs manager
> can handle the duplication (make sure one directory per disk partition is
> used) and return the right directory. By doing this, all the directory/disk
> partition related complexity is hidden and managed and people don't have
> the know the details.
>
> I don't have any strong opinion adding another setting. My feeling is that
> we are not taking the right approach to address the problem here.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/bookkeeper/pull/189#issuecomment-311486531>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAChrujqc3KnxxpUHY4AZ82vXNU7zblzks5sIW_EgaJpZM4N4EFC>
> .
>
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
---
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.
---