Fedora: be more efficient for detecting brick mux setting#129
Open
obnoxxx wants to merge 1 commit intogluster:masterfrom
Open
Fedora: be more efficient for detecting brick mux setting#129obnoxxx wants to merge 1 commit intogluster:masterfrom
obnoxxx wants to merge 1 commit intogluster:masterfrom
Conversation
Previously, we were grep-ing through the output of gluster volume info for the value of the cluster.brick-multiplex setting. But it is highly inefficient to use `gluster volume list` to obtain a global setting. With many volumes, this can take unnecessarily long. Furthermore, since this is invoked right after the start of glusterd, glusterd may not be done initializing. This patch changes the retrieval of the brick multiplex setting to use the command `gluster volume get all cluster.brick-multiplex` which is exactly made for this purpose and won't get the whole list of volumes. Signed-off-by: Michael Adam <obnox@redhat.com>
Author
|
CAVEAT: needs to be tested |
Author
|
@atinmu this should roughly be what we discussed on chat today. Please take a look! |
atinmu
reviewed
Feb 21, 2019
atinmu
left a comment
atinmu
left a comment
There was a problem hiding this comment.
Approach looks good and the same as what we discussef. I’d request the functional correctness of the script to be validated by other maintainers.
nixpanic
reviewed
Feb 21, 2019
nixpanic
left a comment
nixpanic
left a comment
There was a problem hiding this comment.
Looks good to me, but the difference between off/disable needs to be explained.
| GLUSTER_BRICKMULTIPLEX_SETTING="$( \ | ||
| gluster --mode=script volume get all cluster.brick-multiplex \ | ||
| | grep '^cluster.brick-multiplex' \ | ||
| | awk '{print $2}')" |
There was a problem hiding this comment.
you can skip the grep if you do this:
awk '/^cluster.brick-multiplex/ {print $2}'
| [nN] | [nN][Oo] ) | ||
| gluster v info | grep 'cluster.brick-multiplex: off' > $GLUSTERFS_LOG_CONT_DIR/brickmultiplexing | ||
| if [[ ${?} == 0 ]]; then | ||
| if [[ "${GLUSTER_BRICKMULTIPLEX_SETTING}" == "disable" ]]; then |
There was a problem hiding this comment.
Before the check was for "off", now it is "disable"? What does gluster --mode=script volume get all cluster.brick-multiplex really return?
|
@obnoxxx ping :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, we were grep-ing through the output of gluster volume info
for the value of the cluster.brick-multiplex setting. But it is highly
inefficient to use
gluster volume listto obtain a global setting.With many volumes, this can take unnecessarily long. Furthermore, since
this is invoked right after the start of glusterd, glusterd may not be
done initializing.
This patch changes the retrieval of the brick multiplex setting
to use the command
gluster volume get all cluster.brick-multiplexwhich is exactly made for this purpose and won't get the whole list
of volumes.