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.

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

Jump to bottom

feat: add databaseMirroringPartner event handler #1671

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+12 −4
Open

feat: add databaseMirroringPartner event handler #1671

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24f7e6b
added databaseMirroringPartner event emitter
nick-w-nick Oct 31, 2024
4891a41
added onDatabaseMirroringPartner handler methods
nick-w-nick Oct 31, 2024
5b6e12d
removed unnecessary database mirroring event handlers
nick-w-nick Nov 1, 2024
f209f0e
re-added database mirroring handler for login7tokenhandler
nick-w-nick Nov 1, 2024
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
4 changes: 4 additions & 0 deletions src/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1920,6 +1920,10 @@ class Connection extends EventEmitter {
* @private
*/
emit(event: 'databaseChange', databaseName: string): boolean
/**
* @private
*/
emit(event: 'databaseMirroringPartner', partnerInstanceName: string): boolean
/**
* @private
*/
Expand Down
16 changes: 12 additions & 4 deletions src/token/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@
this.connection.emit('databaseChange', token.newValue);
}

onDatabaseMirroringPartner(token: DatabaseMirroringPartnerEnvChangeToken) {
this.connection.emit('databaseMirroringPartner', token.newValue);

Check warning on line 170 in src/token/handler.ts

View check run for this annotation

Codecov / codecov/patch

src/token/handler.ts#L170 

Added line #L170 was not covered by tests
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that the database mirroring partner event is sent by the server when the initial SQL setup query is sent by tedious?

This is... surprising. 🤔

The MS-TDS specification states that:

Type 13 (Database Mirroring) is sent in response to a LOGIN7 message whenever connection is requested to a database that it is being served as primary in real-time log shipping. The ENVCHANGE stream reflects the name of the partner node of the database that is being log shipped.

I'm wondering if there's some other issue in the sequencing of how we handle events and which state the handling happens in. Because based just on the specification, this event should not be send by the server in a response to the initial sql query we send.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arthurschreiber You're actually 100% correct, I mistakenly had added the event emitters in each of them during debugging but hadn't removed them to determine if it was a specific handler or not.

I've removed the unnecessary event handler from the InitialSqlTokenHandler class and verified that the event is still handled correctly.

Thanks!

onLanguageChange(token: LanguageEnvChangeToken) {
this.connection.emit('languageChange', token.newValue);
}
Expand Down Expand Up @@ -284,6 +288,10 @@
this.connection.emit('databaseChange', token.newValue);
}

onDatabaseMirroringPartner(token: DatabaseMirroringPartnerEnvChangeToken) {
this.connection.emit('databaseMirroringPartner', token.newValue);

Check warning on line 292 in src/token/handler.ts

View check run for this annotation

Codecov / codecov/patch

src/token/handler.ts#L292 

Added line #L292 was not covered by tests
}

onLanguageChange(token: LanguageEnvChangeToken) {
this.connection.emit('languageChange', token.newValue);
}
Expand Down Expand Up @@ -355,10 +363,6 @@
onPacketSizeChange(token: PacketSizeEnvChangeToken) {
this.connection.messageIo.packetSize(token.newValue);
}

onDatabaseMirroringPartner(token: DatabaseMirroringPartnerEnvChangeToken) {
// Do nothing
}
}
Comment on lines 361 to 362
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this handler to be right below the onDatabaseChange event handler for better ordering and to keep the event next to all of the other ENVCHANGE events.


/**
Expand Down Expand Up @@ -406,6 +410,10 @@
this.connection.emit('databaseChange', token.newValue);
}

onDatabaseMirroringPartner(token: DatabaseMirroringPartnerEnvChangeToken) {
this.connection.emit('databaseMirroringPartner', token.newValue);

Check warning on line 414 in src/token/handler.ts

View check run for this annotation

Codecov / codecov/patch

src/token/handler.ts#L414 

Added line #L414 was not covered by tests
}

onLanguageChange(token: LanguageEnvChangeToken) {
this.connection.emit('languageChange', token.newValue);
}
Expand Down
Loading