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

[FAI-13688] Run source airbyte connector docker container #101

Merged
merged 7 commits into from
Jan 14, 2025

Conversation

jeniii
Copy link
Collaborator

@jeniii jeniii commented Jan 7, 2025

Description

Provide description here

This PR doesn't include process the output of the source connector.

@jeniii jeniii marked this pull request as ready for review January 13, 2025 18:11
AutoRemove: true,
Init: true,
},
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confirmed the cpu / mem / log settings.
A screenshot of docker inspect the container
Screenshot 2025-01-10 at 11 58 32

airbyte-local-cli-nodejs/src/docker.ts Outdated Show resolved Hide resolved

// Write the container ID to the cidfile
const cidfilePath = `tmp-${timestamp}-src_cid`;
writeFileSync(cidfilePath, container.id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i followed the current script to have a cid file. is it actually helpful?

maxMemory?: number; // unit: MB
maxCpus?: number; // unit: CPU
maxLogSize?: string; // default: 10m (10MB)
additionalOptions?: any;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the dockerode lib doesn't take the docker options as a string instead it take a typed structure. i think they are corresponding to this https://docs.docker.com/reference/api/engine/version/v1.43/#tag/Container/operation/ContainerCreate

so i break down to a few variables and have an free form to take options that are not pre-defined by us. I would assume this won't be used often but might be useful when we or users need debugging

});
// Attach the stderr to termincal stderr, and stdout to the output stream
const stream = await container.attach({stream: true, stdout: true, stderr: true});
container.modem.demuxStream(stream, stdoutStream, process.stderr);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

stdout -> captured and returned in this function
stderr -> terminal

airbyte-local-cli-nodejs/src/docker.ts Outdated Show resolved Hide resolved
@@ -14,6 +14,7 @@ export const SRC_CONFIG_FILENAME = `${FILENAME_PREFIX}_src_config.json`;
export const DST_CONFIG_FILENAME = `${FILENAME_PREFIX}_dst_config.json`;
export const SRC_CATALOG_FILENAME = `${FILENAME_PREFIX}_src_catalog.json`;
export const DST_CATALOG_FILENAME = `${FILENAME_PREFIX}_dst_catalog.json`;
export const DEFAULT_STATE_FILE = 'state.json';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should make the default state file name something that includes the src and dst image names, so it doesn't just keep overwriting the same state.json file every time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i thought we replace the state file every time?

this state file is in the temp folder that has differ name every run. and it will replace the state file that is outside the temp folder at the end of the run

@jeniii jeniii merged commit 37d9c6d into main Jan 14, 2025
6 checks passed
@jeniii jeniii deleted the jg/run-src-sync branch January 14, 2025 19:04
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants