Skip to content

Conversation

@TCeason
Copy link
Collaborator

@TCeason TCeason commented Nov 7, 2025

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

prevent masking/row access policy name conflicts

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-refactor this PR changes the code base without new features or bugfix label Nov 7, 2025
@TCeason TCeason requested a review from drmingdrmer November 7, 2025 12:19
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

@drmingdrmer reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions


src/meta/api/src/data_mask_api_impl.rs line 105 at r1 (raw file):

                )
                .into());
            }

This function looks strange:

  • fetch_id can be moved out from the transaction-retry-loop
  • it does not need a loop to retry: just one transaction trial would be enough.

Code quote:

            let row_access_name_ident = RowAccessPolicyNameIdent::new(
                name_ident.tenant().clone(),
                name_ident.data_mask_name().to_string(),
            );
            if self.get_pb(&row_access_name_ident).await?.is_some() {
                return Err(AppError::DatamaskAlreadyExists(
                    name_ident.exist_error("name conflicts with an existing row access policy"),
                )
                .into());
            }

src/meta/api/src/data_mask_api_impl.rs line 112 at r1 (raw file):

            // data mask name -> data mask table id list

            let id = fetch_id(self, IdGenerator::data_mask_id()).await?;

fetch_id can be moved out from the transaction-retry-loop.

Copy link
Collaborator Author

@TCeason TCeason left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)


src/meta/api/src/data_mask_api_impl.rs line 105 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

This function looks strange:

  • fetch_id can be moved out from the transaction-retry-loop
  • it does not need a loop to retry: just one transaction trial would be enough.

Done.


src/meta/api/src/data_mask_api_impl.rs line 112 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

fetch_id can be moved out from the transaction-retry-loop.

Done.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2025

🤖 CI Job Analysis

Workflow: 19193842687

📊 Summary

  • Total Jobs: 23
  • Failed Jobs: 1
  • Retryable: 0
  • Code Issues: 1

NO RETRY NEEDED

All failures appear to be code/test issues requiring manual fixes.

🔍 Job Details

  • linux / check: Not retryable (Code/Test)

🤖 About

Automated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-refactor this PR changes the code base without new features or bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants