keith-turner commented on code in PR #6040:
URL: https://github.com/apache/accumulo/pull/6040#discussion_r2674030686
##########
core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java:
##########
@@ -73,8 +72,25 @@ public class NewTableConfiguration {
private Map<String,String> summarizerProps = Collections.emptyMap();
private Map<String,String> localityProps = Collections.emptyMap();
private final Map<String,String> iteratorProps = new HashMap<>();
+ private final Map<String,String> inheritedIteratorProps = new HashMap<>();
private SortedSet<Text> splitProps = Collections.emptySortedSet();
+ /**
+ * Configures the {@link NewTableConfiguration} with iterators inherited
from the parent
+ * namespace. This is used internally in table creation - no need to call
directly.
+ *
+ * @param props the parent namespace config
+ */
+ public void configureInheritedIteratorProps(Map<String,String> props) {
Review Comment:
If the code is only needed by internal code, then its best to avoid adding
to the public API. Would it be possible to do somethnig like the following
instead in TableOperationsImpl.create() (or maybe even in the manager create
table code, it usually better to do validation on the server side for things
like this rather than trust the client to do it)?
```java
var tableProps = ntc.getProperties();
var namespaceProps =
context.namespaceOperations().getNamespaceProperties(ns);
// TODO throw exception if namespace and table iter props conflict
```
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -812,6 +817,33 @@ public Scanner createScanner(String tableName)
return createScanner(tableName, auths);
}
+ private Consumer<IteratorSetting> getScanIteratorValidator(Callable<String>
tableNameGetter) {
+ return (givenIter) -> {
+ try {
+ tableOperations().checkIteratorConflicts(tableNameGetter.call(),
givenIter,
Review Comment:
Every time an iterator is added to a scanner on the client side this code
wil make an RPC to get the tables full config and then check it. This could be
expensive and slow down code that is current creating a scanner and adding lots
of iterators.
Instead of doing all this validation client side, we could potentially do it
server side and not make any changes to the client side code. We are already
passing iterators to servers for the scan RPCs, we could potentialy check for
the conflicts in the following code and throw an exception if any are seen.
https://github.com/apache/accumulo/blob/b3c938ac0c27b49542e0fe5e729b6b889db12c61/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java#L244-L249
Not sure if that is workable, would be good to rule this in or out as an
option. Would be nice to efficiently do the check leveraging the existing
merge code if possible.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java:
##########
@@ -199,29 +215,51 @@ public Map<String,String> getProperties() {
// check the properties for conflicts with default iterators
var defaultIterSettings =
IteratorConfigUtil.getInitialTableIteratorSettings();
// if a default prop already exists, don't want to consider that a
conflict
- var noDefaultsPropMap = new HashMap<>(propertyMap);
- noDefaultsPropMap.entrySet().removeIf(entry ->
initTableProps.get(entry.getKey()) != null
- && initTableProps.get(entry.getKey()).equals(entry.getValue()));
- defaultIterSettings.forEach((setting, scopes) -> {
+ for (var defaultIterSetting : defaultIterSettings.entrySet()) {
+ var setting = defaultIterSetting.getKey();
+ var scopes = defaultIterSetting.getValue();
try {
- TableOperationsHelper.checkIteratorConflicts(noDefaultsPropMap,
setting, scopes);
+ TableOperationsHelper.checkIteratorConflicts(propertyMap, setting,
scopes);
} catch (AccumuloException e) {
- throw new IllegalStateException(String.format(
+ throw new AccumuloException(String.format(
"conflict with default table iterator: scopes: %s setting: %s",
scopes, setting), e);
}
Review Comment:
In general adding or removing checked exceptions from public API methods
will always result in source and binary incompatibilities. When adding checked
exceptions any calling code that does not already catch or throw the exception
will now fail at compile or runtime. When removing checked exceptions it can
cause code that catches them unnecessarily to fail. So would not want to make
this change in 2.1 or even 4.0. The only way I know to change checked
exceptions cleanly is to deprecate the method and add a new method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
