make dgraph import work across the internet #9456
Open
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
github-actions
bot
added
go
mangalaman93
force-pushed
the
aman/import-bug
branch
from
734c583 to
9f903b8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for streaming p directories in import command by implementing bidirectional streaming for external snapshot operations. The core purpose is to enhance the import functionality to handle streaming of snapshot data to and from p directories more efficiently.
Key changes:
- Converts external snapshot streaming from unidirectional to bidirectional streaming with proper acknowledgment flow
- Adds support for direct p directory streaming in import command with new
-pflag - Improves error handling and logging throughout the streaming process
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| worker/import.go | Implements bidirectional streaming with Send/Recv pattern and enhanced error handling |
| protos/pb.proto | Updates StreamExtSnapshot service to support bidirectional streaming |
| protos/pb/pb_grpc.pb.go | Generated code changes for bidirectional streaming interface |
| dgraph/cmd/dgraphimport/run.go | Adds support for -p flag to directly import from snapshot directories |
| dgraph/cmd/dgraphimport/import_client.go | Implements client-side bidirectional streaming with acknowledgments |
| edgraph/server.go | Updates server-side streaming handlers with better error handling |
| Other files | Build system improvements and test updates |
Comments suppressed due to low confidence (1)
worker/import.go:246
- [nitpick] The error message has inconsistent verb formatting. Change to "failed streamInGroupto send forward request: %w" to match the wrapping pattern used elsewhere or keep it consistent with line 246.
return fmt.Errorf("failed to send forward request: %v", err)
This PR fixes the timeout issue when cloudflare is not happy for just one way data send. It adds application level ACKs to work around the 120s timeout for a HTTP response. Additionally, it adds an argument to take p directory as input for the dgraph import command.
mangalaman93
force-pushed
the
aman/import-bug
branch
from
55d464a to
ab94bf1
Compare
github-actions
bot
added
the
area/integrations
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.
This PR fixes the timeout issue when cloudflare is not happy for just one way data send. It adds application level ACKs to work around the 120s timeout for a HTTP response.
Additionally, it adds an argument to take p directory as input for the dgraph import command.