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.

Sign up for GitHub

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

Jump to bottom

Implement Azure blob replication logic #7308

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Implement Azure blob replication logic #7308

merged 1 commit into from
Jul 10, 2023

Conversation

Neon-White
Copy link
Contributor

@Neon-White Neon-White commented May 24, 2023 •
edited

Explain the changes

  1. Initial implementation of Azure blob log retrieval and parsing
  2. Addition of a unit test for the Azure parser
  3. Replaced the ignore_fn lambda function with the ignore_deletions flag, and moved the handling to the log parsers

TODO

  1. Things marked with NWTODO (Neon White TODO)
  • Tests added

@Neon-White Neon-White added the Compatibility-Azure Blob Compatibility and Namespace over Azure label May 24, 2023
@Neon-White Neon-White force-pushed the read-azure-logs branch 2 times, most recently from 2b90866 to 9d2429f Compare May 25, 2023 23:35
@Neon-White Neon-White mentioned this pull request Jun 19, 2023
Closed
@Neon-White Neon-White force-pushed the read-azure-logs branch 3 times, most recently from b1ffe70 to 6acd7b8 Compare July 4, 2023 08:48
dannyzaken
dannyzaken reviewed Jul 4, 2023
View reviewed changes
src/api/account_api.js Outdated Show resolved Hide resolved
@Neon-White Neon-White mentioned this pull request Jul 4, 2023
Merged
@Neon-White Neon-White force-pushed the read-azure-logs branch 2 times, most recently from 62c6dda to 3398da3 Compare July 5, 2023 20:27
tangledbytes
tangledbytes reviewed Jul 7, 2023
View reviewed changes
src/server/system_services/account_server.js Outdated
@@ -799,7 +822,11 @@ async function update_external_connection(req) {
},
$set: {
"sync_credentials_cache.$.access_key": identity,
"sync_credentials_cache.$.secret_key": encrypted_secret
"sync_credentials_cache.$.secret_key": encrypted_secret,
"sync_credentials_cache.$.azure_log_access_keys.azure_tenant_id": req.rpc_params.azure_log_access_keys.azure_tenant_id || undefined,
Copy link
Member

Choose a reason for hiding this comment

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

Can you please check the behaviour when these fields are set to undefined? Actually I think I noticed a fluke with our encode_json which iterates over all the keys and captures the keys even if the values are set to undefined. I am not sure if that applies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The update logic will be implemented in another PR, so I'll address it there :)

romayalon
romayalon reviewed Jul 10, 2023
View reviewed changes
src/server/system_services/bucket_server.js Outdated Show resolved Hide resolved
src/server/system_services/schemas/replication_configuration_schema.js Show resolved Hide resolved
src/server/system_services/bucket_server.js Outdated Show resolved Hide resolved
src/api/bucket_api.js Show resolved Hide resolved
src/api/bucket_api.js
properties: {
logs_bucket: { type: 'string' },
prefix: { type: 'string' },
endpoint_type: { $ref: '#/definitions/log_replication_endpoint_type' },
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need endpoint type here if this is AWS for sure?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you need to take the endpoint_type outside to the outer layer of this object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it as an optional parameter in case users will want to use it for better clarity, and for consistency with the Azure option - but it serves no functional purpose

src/server/system_services/account_server.js Outdated Show resolved Hide resolved
src/server/system_services/replication_store.js Outdated Show resolved Hide resolved
src/server/bg_services/replication_log_parser.js Outdated Show resolved Hide resolved
src/server/bg_services/replication_log_parser.js Outdated Show resolved Hide resolved
src/server/bg_services/replication_log_parser.js Outdated Show resolved Hide resolved
dannyzaken
dannyzaken reviewed Jul 10, 2023
View reviewed changes
src/server/system_services/account_server.js Outdated Show resolved Hide resolved
src/api/account_api.js Outdated Show resolved Hide resolved
@Neon-White Neon-White force-pushed the read-azure-logs branch 3 times, most recently from 165708d to 9be1d13 Compare July 10, 2023 15:34
@Neon-White
Implement Azure log-based replication optimization
626c88e 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White Neon-White merged commit 7146a89 into noobaa:master Jul 10, 2023
7 checks passed
@Neon-White Neon-White mentioned this pull request Jul 11, 2023
Merged
@romayalon romayalon mentioned this pull request Jul 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romayalon romayalon romayalon left review comments

@tangledbytes tangledbytes tangledbytes left review comments

@dannyzaken dannyzaken dannyzaken approved these changes

Assignees
No one assigned
Labels
Compatibility-Azure Blob Compatibility and Namespace over Azure size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Neon-White @dannyzaken @romayalon @tangledbytes