-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
HIVE-23413: Create a new config to skip all locks #1220
Conversation
@deniskuzZ could you check this out, you already reviewed it on the review board, but I forgat about it, so now created a PR. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@deniskuzZ @pvary Do you still think we need this? If so please review, and I will check if it needs rebase. |
public void testCheckExpectedLocks2NoReadLock() throws Exception { | ||
testCheckExpectedLocks2(false); | ||
} | ||
public void testCheckExpectedLocks2(boolean readLocks) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new row
@@ -727,35 +751,52 @@ private void testCheckExpectedLocks(boolean sharedWrite) throws Exception { | |||
*/ | |||
@Test | |||
public void testCheckExpectedLocks2() throws Exception { | |||
testCheckExpectedLocks2(true); | |||
} | |||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is still needed.
Skimmed through it, and only found nits.
I would be happy if @deniskuzZ could also check.
Thanks,
Peter
Checked, however I'm not sure if it is a good idea to give the end user the option to completely disable the locking. They could end up shooting themselves in the foot. From code perspective, change looks good. |
Administrator could add it to the restricted parameters which can not be set by the user, so a customer can turn off if they feel it is problematic. |
@pvary So what is the conclusion? Should I add it to the restricted list by default? Or will you merge this, as is? |
I would like to see an agreement here. This is more like a democratic process, than autocratic :) I still think this will become useful immediately when someone has a problem with the too restrictive locking. If in doubt we might just add this to HIVE_CONF_RESTRICTED_LIST, so no rouge user can use this unless the administrator explicitly allows the usage of the new config. But this is only me, others might different opinions |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
No description provided.