Skip to content

feat(shell-api): add automerge information to sh.status() MONGOSH-1649 #2422

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.

Sign up for GitHub

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

Merged
gagik merged 6 commits into from
Apr 2, 2025
Merged

feat(shell-api): add automerge information to sh.status() MONGOSH-1649 #2422

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c3f5dd
feat(shell-api): add automerge information to sh.status() MONGOSH-1649
gagik Mar 31, 2025
cfb1c3a
test: add start and stop automerge tests
gagik Mar 31, 2025
721736a
fix: do not show if not explicitly enabled
gagik Apr 1, 2025
5615062
fix: test automerge separately
gagik Apr 1, 2025
4b531d9
fix: do not assume not explicitly enabled by default for 7.0+
gagik Apr 1, 2025
3496b4b
fix: delete automerge settings before checking for omission
gagik Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/shell-api/src/helpers.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ describe('getPrintableShardStatus', function () {
);
expect(status['most recently active mongoses']).to.have.lengthOf(1);
expect(status.autosplit['Currently enabled']).to.equal('yes');
expect(status.automerge['Currently enabled']).to.equal('yes');
expect(status.balancer['Currently enabled']).to.equal('yes');
expect(
status.balancer['Failed balancer rounds in last 5 attempts']
Expand Down
12 changes: 12 additions & 0 deletions packages/shell-api/src/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,14 @@ export async function getPrintableShardStatus(
autosplit === null || autosplit.enabled ? 'yes' : 'no',
};
})(),
(async (): Promise<void> => {
// Is automerge currently enabled, available since >= 7.0
Copy link
Contributor Author

@gagik gagik Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using null scenario to return true to match the mongod behavior. But this is available since 7.0 which means in older versions it'd also similarly say 'yes' which is weird.
The solution to this is to check for the server version and compare with semver but I don't see this being used anywhere

  1. Do we want to introduce this kind of differentiation and hide this field < 7.0?
  2. If yes, what does a server version mean for automerge in sharded cluster? If I understand right it's possible for shards to have i.e. mix of 6.0 and 7.0 Mongo versions, does one get it from i.e. adminDB or something of sort?

Copy link
Collaborator

@addaleax addaleax Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering – what does automerge === null correspond to? Is that the case in which we would want to not print this information?

Copy link
Contributor Author

@gagik gagik Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going off https://github.com/mongodb/mongo/blob/a79ad6aa8b026d511227c4ece3d8c80265578831/src/mongo/shell/utils_sh.js#L264 my guess is that the idea here is that automerge wasn't explicitly enabled but it is enabled by default in 7.x+ so they print it out as such

Copy link
Contributor Author

@gagik gagik Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just confirmed this more or less with start/stop autoMerger is when that'd appear.

We could omit it when the information isn't explicitly set and show only if it has been configured; users looking for it would probably end up starting/stopping automerger so seems reasonable, though a bit of divergence from what we usually do

const automerge = await settingsColl.findOne({ _id: 'automerge' });
result.automerge = {
'Currently enabled':
automerge === null || automerge.enabled ? 'yes' : 'no',
};
})(),
(async (): Promise<void> => {
// Is the balancer currently enabled
const balancerEnabled = await settingsColl.findOne({ _id: 'balancer' });
Expand Down Expand Up @@ -713,6 +721,10 @@ export type ShardingStatusResult = {
autosplit: {
'Currently enabled': 'yes' | 'no';
};
/** Available from 7.0.0 */
automerge: {
'Currently enabled': 'yes' | 'no';
};
balancer: {
'Currently enabled': 'yes' | 'no';
'Currently running': 'yes' | 'no' | 'unknown';
Expand Down
2 changes: 2 additions & 0 deletions packages/shell-api/src/shard.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2419,6 +2419,7 @@ describe('Shard', function () {
'shardingVersion',
'shards',
'autosplit',
'automerge',
'balancer',
'databases',
]);
Expand Down Expand Up @@ -2463,6 +2464,7 @@ describe('Shard', function () {
'shardingVersion',
'shards',
'autosplit',
'automerge',
'balancer',
'databases',
]);
Expand Down