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.

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

Rxdb #1987

Draft
wants to merge 46 commits into
base: main
Choose a base branch
from
Draft

Rxdb #1987

wants to merge 46 commits into from

Conversation

hsmr29
Copy link
Contributor

@hsmr29 hsmr29 commented May 31, 2024

  • Remove unused ThoughtspaceOption properties from init
  • Efficient observe mechanism for useObserveCol
  • Key metrics: Load 1k thoughts, Delete 1k thoughts (I will send new sample data)
  • rxdm-premium deployment strategy
  • Get CI tests to pass
  • freeThought/freeLexeme implementation

@hsmr29 hsmr29 force-pushed the rxdb branch 4 times, most recently from f6ff14d to 0a8ecf4 Compare June 17, 2024 11:11
@@ -272,140 +96,10 @@ export const init = async (options: ThoughtspaceOptions) => {
const websocketUrl = await options.websocketUrl
const cursor = await options.cursor

// generate docKeys for cursor, otherwise replicateThought will fail
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the following from ThoughtspaceOptions and this file. They were used by the replication controller which has been replaced with RxDB replication:

  • accessToken
  • cursor
  • getItem
  • setItem
  • tsid
  • tsidShared
  • websocketUrl

We can discuss the migration of the following thoughtspace options:

  • isThoughtLoaded/isLexemeLoaded - Used to subscribe to remote changes that are visible to the user, i.e. thoughts or lexemes that are loaded in the Redux state.
  • onError - Dispatches an error action when there is a db error.
  • onProgress - Updates the UI with replication progress (entire thoughtspace) and save progress (when local changes have been synced to the server).
  • onThoughtChange - Marks a thought as pending if its parent is pending.
  • onThoughtReplicated - Triggers repairThoughtActionCreator to run data integrity checks on the thought tree structure.
  • onThoughtIDBSynced - Used to dismiss the "Connecting..." message as soon as thoughts are loaded from the local db.
  • onUpdateThoughts - Removes a thought from the Redux state when it has been deleted remotely. Adds a Lexeme when it has been remotely loaded or changed. (Does not check if the Lexeme is currently loaded in Redux state.)

@hsmr29 hsmr29 force-pushed the rxdb branch 2 times, most recently from 0fe9dd0 to bd04524 Compare June 24, 2024 19:21
@hsmr29 hsmr29 force-pushed the rxdb branch 3 times, most recently from 0fc0d3d to c34e43f Compare July 3, 2024 20:01
Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. My only major concern is about race conditions and transaction atomicity, mentioned in this comment. Let's talk about this at our next meeting.

I think we can safely reorganize the file structure now that YJS is completely removed:

  • Combine src/data-prociders/yjs/thoughtspace.ts with src/data-prociders/rxdb/thoughtspace.ts
  • Move everything in src/data-providers/yjs and src/data-providers/rxdb up a level to to src/data-providers.

@@ -0,0 +1,142 @@
import { execSync } from 'child_process'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a summary comment to the top of this script for documentation purposes.

Comment on lines 13 to 18
it('Set the text color using the ColorPicker', async () => {
it.skip('Set the text color using the ColorPicker', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment describing why this test is being skipped.

/**
* Subscribes to a rxdb collection, e.g. RxCollection<RxPermission>.
*/
const useObserveCol = <T>(rxCol: RxCollection<T>): Document<T>[] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this useRxCollection.

return createRxDatabase({
name: `${testPrefix}${DATABASE_NAME}`,
storage: await getRxStorage(),
ignoreDuplicate: isTestEnv,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain ignoreDuplicates and why it is needed when in test mode?

properties: {
id: {
type: 'string',
maxLength: 32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create a constant for this id type.

Also let me know what the reasoning is behind the value 32.

Comment on lines +26 to +28
} catch (error) {
return { error }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If a function can't handle an error, it should throw it up the call stack rather than return it.

Comment on lines 226 to 229
console.info(
'TODO_RXDB: thoughtspace.freeThought - The thought is no longer visible and should be removed from memory. Realtime changes to this thought from other clients no longer need to be subscribed to.',
{ id: docKey },
)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove each of these console.info debugging statements. They were just there to assist in the RxDB conversion.


return foundThoughtDoc.toJSON() as Thought
})
}

/** Pauses replication for higher priority network activity, such as push or pull. */
export const pauseReplication = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have the ability to pause replication in RxDB? If not, we can remove pauseReplication and startReplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can pause replication in RxDB.

@@ -435,876 +93,167 @@ export const init = async (options: ThoughtspaceOptions) => {
* Methods
**********************************************************************/

/** Updates a yjs thought doc. Converts childrenMap to a nested Y.Map for proper children merging. Resolves when transaction is committed and IDB is synced (not when websocket is synced). */
// NOTE: Ids are added to the thought log in updateThoughts for efficiency. If updateThought is ever called outside of updateThoughts, we will need to push individual thought ids here.
/** Updates a thought in the db. */
export const updateThought = async (id: ThoughtId, thought: Thought): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned about the possibility of race conditions in updateThought due to all the awaits. With YJS, changes are synchronous. Here, between the await getThoughtById(id), await getLexemeById(lexemeKey), and await thoughtCollection.findOne, it seems like there are many opportunities for the data to have changed in between awaited functions, resulting in inconsistencies.

What's preventing the data in the db from changing in between awaits? How to perform the update as an atomic transaction?

Comment on lines 290 to 286
...Object.entries(thoughtUpdates || {}).map(([id, thought]) => updateThought(id as ThoughtId, thought)),
...Object.entries(lexemeUpdates || {}).map(([key, lexeme]) => updateLexeme(key, lexeme)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these updates be batched for better performance?

@raineorshine
Copy link
Contributor

Also, what is this? It shows up in the console when I clear the local state and refresh.

image

hsmr29 added 22 commits July 22, 2024 17:37
- Destroy Rx database when db.clear method is called
- replicateThought
- replicateChildren
- replicateLexeme
- rxdb-server: Create RxDB server to replicate with the WebRTC plugin
- rxdb/thoughtspace: Initial implementation for replication with the WebRTC controller, Add a simple custom conflict handler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants