Added ScanServer property for allowed tables#6146
Added ScanServer property for allowed tables#6146dlmarion wants to merge 4 commits intoapache:2.1from
Conversation
Added property that allows user to configure which tables are allowed to be scanned by clients at the ScanServer group level. Property defaults to allowing all non-accumulo namespace tables. Closes apache#6123
|
I only perform the allowed table check in startScan/startMultiScan and did not add the checks into the continue methods. Doing so would cause an in-progress scan to fail on the property change. |
| @Experimental | ||
| SSERV_SCAN_ALLOWED_TABLES("sserver.scan.allowed.tables.group.", null, PropertyType.PREFIX, | ||
| "A regular expression that determines which tables are allowed to be scanned for" | ||
| + " servers in the specified group. The property name should end with the scan server" |
There was a problem hiding this comment.
In 4.0, with the changes in #5749 would not need to group name in the property.
There was a problem hiding this comment.
correct. There are probably other changes when merging in 4.0 as well.
|
|
||
| // Visible for testing | ||
| protected boolean isAllowed(TableId tid) { | ||
| boolean result = allowedTables.containsKey(tid); |
There was a problem hiding this comment.
Need to differentiate between not existing in the map and a table id is known to not be allowed. Seems like if a table is not allowed it will constantly regenrate the entire cache for all tables. Could make the value in the map a boolean, then can know all three states of allowed, not allowed, unknown.
There was a problem hiding this comment.
Good idea, I implemented this in 881b810. I had to change the IT to wait for the server to notice the property change.
| // is a race condition in table creation and | ||
| // scan | ||
| updateAllowedTables(true); | ||
| result = allowedTables.get(tid); |
There was a problem hiding this comment.
Its possible the table id still may not exist in the map after the update call because that only works on known tables. A client could pass a table id to the scan server that no longer exists. Could do the following to protect against this and avoid a NPE when trying to convert null to a boolean primitave.
| result = allowedTables.get(tid); | |
| result = allowedTables.getOrDefault(tid, false); |
There was a problem hiding this comment.
I approached this differently in f3ca130, want to get your thoughts on it.
| throw new TException( | ||
| "Only the system user can perform eventual consistency scans on the root and metadata tables"); | ||
| if (!isAllowed(extent.tableId())) { | ||
| throw new TException("Scan of table " + extent.tableId() + " disallowed by property: " |
There was a problem hiding this comment.
This is a preexisting problem. Throwing TException will probably result in a TappException on the client side. Need to throw a thrift exception defined on the RPC to get anything meaningful on the client side.
| LOG.error( | ||
| "Property {} contains an invalid regular expression. Property value: {}. Using default pattern: {}", | ||
| propName, allowedTableRegex, DEFAULT_SCAN_ALLOWED_PATTERN); | ||
| allowedTablePattern = Pattern.compile(DEFAULT_SCAN_ALLOWED_PATTERN); |
There was a problem hiding this comment.
Could fail all scans here (put false for all tables in the map) instead of falling back to the default. Falling back to the default may hide that the system is not working as configured/intended.
| TableId tid = e.getValue(); | ||
| Matcher m = p.matcher(tname); | ||
| if (m.matches()) { | ||
| LOG.debug("Table {} can now be scanned via this ScanServer", tname); |
There was a problem hiding this comment.
Will the log every second for every table? If so should probably be trace.
There was a problem hiding this comment.
My plan is to adjust the logging before I merge. Leaving it as debug for now in case of testing issues.
| scanner.setRange(new Range()); | ||
| scanner.setConsistencyLevel(ConsistencyLevel.EVENTUAL); | ||
| scanner.setExecutionHints(Map.of("scan_type", "use_group1")); | ||
| Wait.waitFor(() -> Iterables.size(scanner) == 100); |
There was a problem hiding this comment.
This does not seem to check for failure
2 participants
Added property that allows user to configure which tables are allowed to be scanned by clients at the ScanServer group level. Property defaults to allowing all non-accumulo namespace tables.
Closes #6123