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
max number of breakout rooms is increased #10154
Conversation
breakoutRoomLimit parameter was introduced to controll max number of breakout rooms in private/config/settings.yml.
I have sent a signed CLA to ffdixon@bigbluebutton.org |
Is there a reason "30" was used here? If this is changed a technically useful limit ought to be selected, not one for a particular use case. How about 64? Or 99? |
30 is just a proposition for a new default value. Since it is not hard-coded anymore, admins can set limit to the values they need in a config file. I have tested 99 shortly and it works fine, except that a drop-down list with 99 entries looks a bit strange. |
@@ -34,6 +34,7 @@ public: | |||
duration: 4000 | |||
remainingTimeThreshold: 30 | |||
remainingTimeAlertThreshold: 1 | |||
breakoutRoomLimit: 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
breakoutRoomLimit: 30 | |
breakoutRoomLimit: 8 |
I recommend keeping the old value as the default value. From what I know it was a good fit for the vast majority of use cases. Also a large number like 30 suggests a use case different than the standard "breaking into groups for discussion"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that in a University setting many users may want to have a higher limit. However, the most important thing for me is to make it possible (please merge!), not the default number as long as we can adjust it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 is fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if I set it to 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIN_BREAKOUT_ROOMS = 2 constant is still hard-coded, so if you set limit to 1 or a smaller number, you will not be able to create any breakout rooms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if we still have a hardcoded bottom cap doesn't make sense to validate the max number to be at least the minimum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, i have added a check for that.
Last time this was proposed there was an issue with the conference numbers generated for each breakout room. Is this resolved? Simply increasing the number in the UI won't work if the server-side cannot create or handle the rooms accordingly. |
Co-authored-by: Anton Georgiev <antobinary@users.noreply.github.com>
It is resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise looks better but those commit messages are not good. It could be much better if all those Update <file>
commits were squashed into one.
5159841
to
90f50bf
Compare
Sorry for that. It is now corrected. |
Please @pedrobmarin could this be reviewed and cleared soon? University terms are about to start around the world and this is an important change. Thanks! |
@ned64 this work is targeting the |
Dear @pedrobmarin That's a real pity! The change seems small (LOC) but has a great impact in the year of distance learning. I know some people who use a proprietary solution because of exactly this limit. Is there a way to speed this up towards a released version? That would be brilliant! |
Closing this one since we merged this at 2.2 |
Excellent - thanks very much!!! |
For as much as I look around, I don't find clear instructions as to which config file needs to be updated in the server. |
|
Will try that out again in case I made a mistake before and feeback. |
public:
Result: Still getting 8 breakout rooms on the html5client. |
...
You do not need to add this parameter, since it is already present in config file. Just change the value. |
@mvasylenko According to It wasn't there and I added it to the top as indicated in my previous message.
/usr/share/meteor/bundle/programs/server/assets/app/config/settings.yml (HTML5 client) I am not sure what could be happening since as per your indication the setting should be there from 2.2.24 Still, even when entered the situation hasn't changed. Any ideas? |
7ba421c was merged into stable branch (v2.2.x-release). It is not ported to develop branch yet. |
I wasn't the one installing BBB in the server. You mean to say that we don't have the proper version installed. |
I managed to spin a instance in AWS and I can confirm all went as you indicated @mvasylenko. The setting was in the yml file and modifying it worked well. |
What does this PR do?
Max number of breakout rooms is increased to 30.breakoutRoomLimit parameter was introduced to controll max number of breakout rooms in private/config/settings.yml.
Minor UI bug fixed (room number was truncated to single digit).
Closes Issue(s)
closes #8657
Motivation
We have a demand for more breakout rooms to work in smaller groups.