fix(y-mongodb.js): Prevent service disruption due to failed data updates #24
Conversation
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
- In `src/y-mongodb.js`: - Enhanced the error handling for `Y.applyUpdate()` failures: If an exception occurs while attempting to apply any update, not only log a warning but also trigger a safety mechanism to halt the current document processing flow. This prevents potential data inconsistencies or more severe system faults. - Added an urgent notification mechanism when critical update application failures are detected, allowing the operations team to respond promptly. - Updated logging to provide more detailed information, including but not limited to the type of error, the name of the affected document, and specific update content, aiding in quick diagnosis of the issue. This fix aims to enhance system robustness and reliability, particularly in handling anomalies from external data sources, preventing localized errors from escalating into widespread service outages.
- 将包名从 "y-mongodb-provider" 改为 "y-mongodb-provider-crashsafe" - 更新 GitHub仓库 URL 和 issue 跟踪链接 - 修改包的 homepage URL
- In `src/y-mongodb.js`: - Fixed an issue where under certain conditions documents were not flushed correctly. The previous condition `applyNum === updates.length - 1` did not ensure that a flush would occur when `updates.length` was exactly equal to `this.flushSize`. The condition has been corrected to `applyNum === updates.length` to ensure documents are flushed after all updates are applied. Additionally, modifications in `package-lock.json` include: - Renaming the project from `y-mongodb-provider` to `y-mongodb-provider-crashsafe` to reflect enhanced crash safety.
… messages - In `src/y-mongodb.js`: - Corrected the condition for flushing documents after applying updates. Now ensures that a flush occurs only when all updates are successfully applied (`applyNum === updates.length`), addressing an off-by-one error in the previous logic. - Simplified the warning message when failing to apply an update by removing redundant information. Additionally, modifications in `package.json` include: - Updated the version from `0.2.0` to `0.2.2`, reflecting the bug fixes and improvements made in this commit.
MaxNoetzold
requested changes
May 21, 2025
| { | ||
| "name": "y-mongodb-provider", | ||
| "version": "0.2.0", | ||
| "name": "y-mongodb-provider-crashsafe", |
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/MaxNoetzold/y-mongodb-provider.git" | ||
| "url": "git+https://github.com/lukenc/y-mongodb-provider.git" |
| "license": "MIT", | ||
| "bugs": { | ||
| "url": "https://github.com/MaxNoetzold/y-mongodb-provider/issues" | ||
| "url": "https://github.com/lukenc/y-mongodb-provider/issues" |
| "src/*" | ||
| ], | ||
| "homepage": "https://github.com/MaxNoetzold/y-mongodb-provider#readme", | ||
| "homepage": "https://github.com/lukenc/y-mongodb-provider#readme", |
| return this._transact(docName, async (db) => { | ||
| const updates = await U.getMongoUpdates(db, docName); | ||
| const ydoc = new Y.Doc(); | ||
| let applyNum = 0; |
There was a problem hiding this comment.
this variable doesnt say what it does, it needs some kind of renaming
| Y.applyUpdate(ydoc, updates[i]); | ||
| try { | ||
| Y.applyUpdate(ydoc, updates[i]); | ||
| } catch (e) { |
There was a problem hiding this comment.
you catch all errors now and remove the option for error handling at later stages at all. This is probably a bad idea, but I am not super sure if error handling was an option anyways because of the transact wrapper.
However, I am even wondering if this makes any sense. If an error is thrown when applying an update, the flush later on wont be happening without your changes, so it doesnt change anything but just adds logging?
src/y-mongodb.js
Outdated
| }); | ||
| if (updates.length > this.flushSize) { | ||
| if (updates.length > this.flushSize && applyNum === updates.length) { | ||
| // 情况1: 更新数量超过阈值并且全部应用成功，执行文档刷新 |
There was a problem hiding this comment.
I only want english comments here, sorry
src/y-mongodb.js
Outdated
Comment on lines
115
to
125
| if (updates.length > this.flushSize && applyNum === updates.length) { | ||
| // 情况1: 更新数量超过阈值并且全部应用成功，执行文档刷新 | ||
| await U.flushDocument(db, docName, Y.encodeStateAsUpdate(ydoc), Y.encodeStateVector(ydoc)); | ||
| } else if (applyNum === updates.length) { | ||
| /* empty */ | ||
| } else if (applyNum !== updates.length) { | ||
| // 情况3: 无法应用所有更新，记录警告 | ||
| console.warn( | ||
| `Failed to apply all updates to document "${docName}". Applied ${applyNum}/${updates.length} updates.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
These if/else blocks can be easily simplified:
Suggested change
| if (updates.length > this.flushSize && applyNum === updates.length) { | |
| // 情况1: 更新数量超过阈值并且全部应用成功，执行文档刷新 | |
| await U.flushDocument(db, docName, Y.encodeStateAsUpdate(ydoc), Y.encodeStateVector(ydoc)); | |
| } else if (applyNum === updates.length) { | |
| /* empty */ | |
| } else if (applyNum !== updates.length) { | |
| // 情况3: 无法应用所有更新，记录警告 | |
| console.warn( | |
| `Failed to apply all updates to document "${docName}". Applied ${applyNum}/${updates.length} updates.`, | |
| ); | |
| } | |
| if (updates.length > this.flushSize && ) { | |
| if (applyNum === updates.length) { | |
| await U.flushDocument(db, docName, Y.encodeStateAsUpdate(ydoc), Y.encodeStateVector(ydoc)); | |
| } else { | |
| console.warn( | |
| `Failed to apply all updates to document "${docName}". Applied ${applyNum}/${updates.length} updates.`, | |
| ); | |
| } | |
| } |
… messages - In `src/y-mongodb.js`: - Corrected the condition for flushing documents after applying updates. Now ensures that a flush occurs only when all updates are successfully applied (`applyNum === updates.length`), addressing an off-by-one error in the previous logic. - Simplified the warning message when failing to apply an update by removing redundant information. Additionally, modifications in `package.json` include: - Updated the version from `0.2.0` to `0.2.2`, reflecting the bug fixes and improvements made in this commit.
- Implemented `ensureIndexes` method in `src/mongo-adapter.js` to create indexes on `docName` and `clock` fields for a specified collection. - Updated `src/utils.js` to call `ensureIndexes` on first write, ensuring indexes are created only once.
- 在 package.json 中将版本号从 0.2.3 修改为 0.2.4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
src/y-mongodb.js:Y.applyUpdate()failures: If an exception occurs while attempting to apply any update, not only log a warning but also trigger a safety mechanism to halt the current document processing flow. This prevents potential data inconsistencies or more severe system faults.This fix aims to enhance system robustness and reliability, particularly in handling anomalies from external data sources, preventing localized errors from escalating into widespread service outages.