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

Fix mongo db client to use GridFS API on 16MB exceed file #1077

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
68 changes: 58 additions & 10 deletions server/backend/database/mongo/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@
"context"
"errors"
"fmt"
"log"
"strings"
gotime "time"

"go.mongodb.org/mongo-driver/bson"
"go.mongodb.org/mongo-driver/bson/primitive"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/gridfs"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/readpref"

Expand Down Expand Up @@ -1068,21 +1070,67 @@
docRefKey types.DocRefKey,
doc *document.InternalDocument,
) error {
// 스냅샷 생성
snapshot, err := converter.SnapshotToBytes(doc.RootObject(), doc.AllPresences())
if err != nil {
return err
}

if _, err := c.collection(ColSnapshots).InsertOne(ctx, bson.M{
"project_id": docRefKey.ProjectID,
"doc_id": docRefKey.DocID,
"server_seq": doc.Checkpoint().ServerSeq,
"lamport": doc.Lamport(),
"version_vector": doc.VersionVector(),
"snapshot": snapshot,
"created_at": gotime.Now(),
}); err != nil {
return fmt.Errorf("insert snapshot: %w", err)
// 16MB 이상이면 GridFS에 저장
const maxSnapshotSize = 16 * 1024 * 1024 // 16MB
xx10222 marked this conversation as resolved.
Show resolved Hide resolved
if len(snapshot) > maxSnapshotSize {
log.Println("16MB over!!!")
xx10222 marked this conversation as resolved.
Show resolved Hide resolved

db := c.client.Database(c.config.YorkieDatabase)

// GridFS 버킷 생성
bucket, err := gridfs.NewBucket(db) // MongoDB의 c.db는 데이터베이스 객체
if err != nil {
return fmt.Errorf("failed to create GridFS bucket: %w", err)
}

// GridFS에 파일 업로드
uploadStream, err := bucket.OpenUploadStream(fmt.Sprintf("%s_snapshot", docRefKey.DocID))
if err != nil {
return fmt.Errorf("failed to open GridFS upload stream: %w", err)
}
defer uploadStream.Close()

Check failure on line 1097 in server/backend/database/mongo/client.go

View workflow job for this annotation

GitHub Actions / build

Error return value of `uploadStream.Close` is not checked (errcheck)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle error returned by uploadStream.Close()

The deferred call to uploadStream.Close() does not check for errors. Since Close() can return an error, it's important to handle it to ensure resources are properly released and any potential errors are captured.

Apply this diff to handle the error:

-    		defer uploadStream.Close()
+    		defer func() {
+    			if err := uploadStream.Close(); err != nil {
+    				log.Printf("Failed to close GridFS upload stream: %v", err)
+    			}
+    		}()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defer uploadStream.Close()
defer func() {
if err := uploadStream.Close(); err != nil {
log.Printf("Failed to close GridFS upload stream: %v", err)
}
}()
🧰 Tools
🪛 GitHub Check: build

[failure] 1097-1097:
Error return value of uploadStream.Close is not checked (errcheck)


// 스냅샷 데이터를 GridFS에 저장
_, err = uploadStream.Write(snapshot)
if err != nil {
return fmt.Errorf("failed to write to GridFS: %w", err)
}

// 파일의 ID (GridFS에서 파일을 식별하는 ObjectId)
fileID := uploadStream.FileID

// GridFS에 저장된 파일 ID를 사용하여 문서 삽입
if _, err := c.collection(ColSnapshots).InsertOne(ctx, bson.M{
"project_id": docRefKey.ProjectID,
"doc_id": docRefKey.DocID,
"server_seq": doc.Checkpoint().ServerSeq,
"lamport": doc.Lamport(),
"version_vector": doc.VersionVector(),
"snapshot_file_id": fileID, // GridFS 파일 ID
"created_at": gotime.Now(),
}); err != nil {
return fmt.Errorf("insert snapshot info: %w", err)
}

} else {
// 스냅샷이 16MB 이하일 경우 일반적인 컬렉션에 삽입
if _, err := c.collection(ColSnapshots).InsertOne(ctx, bson.M{
"project_id": docRefKey.ProjectID,
"doc_id": docRefKey.DocID,
"server_seq": doc.Checkpoint().ServerSeq,
"lamport": doc.Lamport(),
"version_vector": doc.VersionVector(),
"snapshot": snapshot,
"created_at": gotime.Now(),
}); err != nil {
return fmt.Errorf("insert snapshot: %w", err)
}
}

return nil
Expand Down
31 changes: 31 additions & 0 deletions server/backend/database/testcases/testcases.go
Original file line number Diff line number Diff line change
Expand Up @@ -1619,3 +1619,34 @@
}
assert.EqualValues(t, expectedKeys, keys)
}

func CreateLargeSnapshotTest(t *testing.T, db database.Database, projectID types.ID) {

Check failure on line 1623 in server/backend/database/testcases/testcases.go

View workflow job for this annotation

GitHub Actions / build

exported: exported function CreateLargeSnapshotTest should have comment or be unexported (revive)
xx10222 marked this conversation as resolved.
Show resolved Hide resolved
t.Run("store and validate large snapshot test", func(t *testing.T) {
ctx := context.Background()
docKey := key.Key(fmt.Sprintf("tests$%s", t.Name()))

clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name())
bytesID, _ := clientInfo.ID.Bytes()
actorID, _ := time.ActorIDFromBytes(bytesID)
docInfo, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)

doc := document.New(docKey)
doc.SetActor(actorID)

largeData := make([]byte, 16*1024*1024+1) // 16MB + 1 byte
for i := range largeData {
largeData[i] = byte('A' + (i % 26)) // A-Z 반복
}

assert.NoError(t, doc.Update(func(root *json.Object, p *presence.Presence) error {
root.SetBytes("largeField", largeData)
return nil
}))

docRefKey := docInfo.RefKey()

// 스냅샷 생성 및 오류 확인
err := db.CreateSnapshotInfo(ctx, docRefKey, doc.InternalDocument())
assert.NoError(t, err)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for the stored snapshot.

The test only verifies that the snapshot was created without error. Consider adding validation to ensure the stored snapshot matches the original document.

 err := db.CreateSnapshotInfo(ctx, docRefKey, doc.InternalDocument())
 assert.NoError(t, err)
+
+// Retrieve and validate the stored snapshot
+snapshot, err := db.FindClosestSnapshotInfo(ctx, docRefKey, 0, false)
+assert.NoError(t, err)
+assert.NotNil(t, snapshot)
+
+// Verify the snapshot contains the large field
+var storedDoc document.InternalDocument
+err = snapshot.LoadSnapshot(&storedDoc)
+assert.NoError(t, err)
+assert.Equal(t, largeData, storedDoc.Root().GetBytes("largeField"))

Committable suggestion skipped: line range outside the PR's diff.

})
}
4 changes: 4 additions & 0 deletions test/complex/mongo_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,8 @@ func TestClientWithShardedDB(t *testing.T) {
assert.Equal(t, docInfo1.Key, result.Key)
assert.Equal(t, docInfo1.ID, result.ID)
})

t.Run("CreateLargeSnapshotTest test", func(t *testing.T) {
testcases.CreateLargeSnapshotTest(t, cli, dummyProjectID)
})
}
Loading