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

PHOENIX-6555 Wait for permissions to sync in Permission tests #1335

Closed
wants to merge 1 commit into from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Oct 19, 2021

This copies and used the permission sync code from the HBase tests.

@stoty
Copy link
Contributor Author

stoty commented Oct 20, 2021

The tests are successful, but they couldn't update the PR.

@virajjasani
Copy link
Contributor

Thanks @stoty, will get to review this soon.

Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

Few nits, +1 overall

Comment on lines +1093 to +1097
//Initialize Phoenix to avoid timeouts later
try (Connection conn = getConnection();
Statement stmt = conn.createStatement();) {
stmt.execute("select * from system.catalog");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

better


@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public abstract class BasePermissionsIT extends BaseTest {

private static final Logger LOGGER = LoggerFactory.getLogger(BasePermissionsIT.class);

private static final int WAIT_TIME = 10000;

private static String SUPER_USER = System.getProperty("user.name");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this change, but can be made final if you would like.

}
}
}
});
}

void grantPermissions(String groupEntry, Permission.Action... actions) throws IOException, Throwable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like IOException is not required, again not relevant but good to update

public Void call() throws Exception {
try {
for (String table : tablesToGrant) {
AccessControlClient.revoke(getUtility().getConnection(), TableName.valueOf(table), toUser, null, null, actions);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: checkstyle line len might complain?

@stoty stoty closed this Oct 22, 2021
@stoty
Copy link
Contributor Author

stoty commented Oct 22, 2021

Thanks @virajjasani , I've addressed your points before commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants