kevinrr888 commented on code in PR #5490:
URL: https://github.com/apache/accumulo/pull/5490#discussion_r2052732655
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -292,15 +280,54 @@ public Iterator<Entry<Key,Value>> iterator() {
EMPTY_BYTES, tableConf);
}
+ ClientServiceEnvironmentImpl senv = new
ClientServiceEnvironmentImpl(null) {
Review Comment:
Might be nicer to avoid anon class
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -292,15 +280,54 @@ public Iterator<Entry<Key,Value>> iterator() {
EMPTY_BYTES, tableConf);
}
+ ClientServiceEnvironmentImpl senv = new
ClientServiceEnvironmentImpl(null) {
+
+ @Override
+ public String getTableName(TableId tableId) throws
TableNotFoundException {
+ throw new IllegalStateException("TableId not known in RFileScanner");
Review Comment:
```
private static final String errorMsg =
"This scanner is unrelated to any table or accumulo instance;"
+ " it operates directly on files. Therefore, it can not support
this operation.";
```
Is the error message used in #4816, could keep this or something similar
since it's more descriptive
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -292,15 +280,54 @@ public Iterator<Entry<Key,Value>> iterator() {
EMPTY_BYTES, tableConf);
}
+ ClientServiceEnvironmentImpl senv = new
ClientServiceEnvironmentImpl(null) {
+
+ @Override
+ public String getTableName(TableId tableId) throws
TableNotFoundException {
+ throw new IllegalStateException("TableId not known in RFileScanner");
+ }
+
+ @Override
+ public <T> T instantiate(String className, Class<T> base)
+ throws ReflectiveOperationException, IOException {
+ return
RFileScanner.class.getClassLoader().loadClass(className).asSubclass(base)
+ .getDeclaredConstructor().newInstance();
+ }
+
+ @Override
+ public <T> T instantiate(TableId tableId, String className, Class<T>
base)
+ throws ReflectiveOperationException, IOException {
+ return instantiate(className, base);
+ }
+
+ @Override
+ public Configuration getConfiguration() {
+ return new ConfigurationImpl(new
ConfigurationCopy(DefaultConfiguration.getInstance()));
+ }
Review Comment:
In #4816, had this method throwing an exception. Don't remember the
specifics, but should make sure this functions as we expect (should it throw an
exception, or return something)
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/OfflineIterator.java:
##########
@@ -345,9 +245,13 @@ private SortedKeyValueIterator<Key,Value>
createIterator(KeyExtent extent,
MultiIterator multiIter = new MultiIterator(readers, extent);
- OfflineIteratorEnvironment iterEnv =
- new OfflineIteratorEnvironment(context, tableId, authorizations,
tableCC, false,
- samplerConfImpl == null ? null :
samplerConfImpl.toSamplerConfiguration());
+ ClientIteratorEnvironment.Builder iterEnvBuilder =
+ new
ClientIteratorEnvironment.Builder().withAuthorizations(authorizations)
+
.withScope(IteratorScope.scan).withTableId(tableId).withClient(context);
+ if (scannerSamplerConfig != null) {
+ iterEnvBuilder.withSamplerConfiguration(scannerSamplerConfig);
+ }
+ IteratorEnvironment iterEnv = iterEnvBuilder.build();
Review Comment:
It seems usage of `tableCC` and `samplerConfImpl` were dropped. Was this
intentional?
##########
core/src/main/java/org/apache/accumulo/core/iterators/ClientIteratorEnvironment.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.iterators;
+
+import java.io.IOException;
+import java.util.Optional;
+
+import org.apache.accumulo.core.client.AccumuloClient;
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.client.SampleNotPresentException;
+import org.apache.accumulo.core.client.sample.SamplerConfiguration;
+import org.apache.accumulo.core.clientImpl.ClientContext;
+import org.apache.accumulo.core.clientImpl.ClientServiceEnvironmentImpl;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope;
+import org.apache.accumulo.core.iteratorsImpl.system.MapFileIterator;
+import org.apache.accumulo.core.security.Authorizations;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+
+public class ClientIteratorEnvironment implements IteratorEnvironment {
+
+ public static class Builder {
+
+ private Optional<IteratorScope> scope = Optional.empty();
+ private boolean isFullMajorCompaction = false;
+ private Optional<Authorizations> auths = Optional.empty();
+ private boolean isUserCompaction = false;
+ private Optional<TableId> tableId = Optional.empty();
+ private Optional<SamplerConfiguration> samplerConfig = Optional.empty();
+ protected Optional<ClientServiceEnvironmentImpl> env = Optional.empty();
+
+ public Builder withScope(IteratorScope scope) {
+ this.scope = Optional.of(scope);
+ return this;
+ }
+
+ public Builder isFullMajorCompaction() {
+ this.isFullMajorCompaction = true;
+ return this;
+ }
+
+ public Builder withAuthorizations(Authorizations auths) {
+ this.auths = Optional.of(auths);
+ return this;
+ }
+
+ public Builder isUserCompaction() {
+ this.isUserCompaction = true;
+ return this;
+ }
+
+ public Builder withTableId(TableId tableId) {
+ this.tableId = Optional.of(tableId);
+ return this;
+ }
+
+ public Builder withSamplerConfiguration(SamplerConfiguration sc) {
+ this.samplerConfig = Optional.of(sc);
+ return this;
+ }
+
+ public Builder withClient(AccumuloClient client) {
+ this.env = Optional.of(new ClientServiceEnvironmentImpl((ClientContext)
client));
+ return this;
+ }
+
+ public ClientIteratorEnvironment build() {
+ return new ClientIteratorEnvironment(this);
+ }
+
+ }
+
+ private static final UnsupportedOperationException UOE =
+ new UnsupportedOperationException("Feature not supported");
+
+ public static final IteratorEnvironment DEFAULT = new Builder().build();
+
+ private final Optional<IteratorScope> scope;
+ private final boolean isFullMajorCompaction;
+ private final Optional<Authorizations> auths;
+ private final boolean isUserCompaction;
+ private final Optional<TableId> tableId;
+ private final Optional<SamplerConfiguration> samplerConfig;
+ private final Optional<ClientServiceEnvironmentImpl> env;
+
+ private ClientIteratorEnvironment(Builder builder) {
+ this.scope = builder.scope;
+ this.isFullMajorCompaction = builder.isFullMajorCompaction;
+ this.auths = builder.auths;
+ this.isUserCompaction = builder.isUserCompaction;
+ this.tableId = builder.tableId;
+ this.samplerConfig = builder.samplerConfig;
+ this.env = builder.env;
+ }
+
+ private ClientIteratorEnvironment(ClientIteratorEnvironment copy) {
+ this.scope = copy.scope.isPresent() ?
Optional.of(copy.scope.orElseThrow()) : Optional.empty();
+ this.isFullMajorCompaction = copy.isFullMajorCompaction;
+ this.auths = copy.auths.isPresent() ?
Optional.of(copy.auths.orElseThrow()) : Optional.empty();
+ this.isUserCompaction = copy.isUserCompaction;
+ this.tableId =
+ copy.tableId.isPresent() ? Optional.of(copy.tableId.orElseThrow()) :
Optional.empty();
+ this.samplerConfig = copy.samplerConfig.isPresent()
+ ? Optional.of(copy.samplerConfig.orElseThrow()) : Optional.empty();
+ this.env = copy.env.isPresent() ? Optional.of(copy.env.orElseThrow()) :
Optional.empty();
+ }
+
+ @Override
+ @Deprecated(since = "2.0.0")
+ public SortedKeyValueIterator<Key,Value> reserveMapFileReader(String
mapFileName)
+ throws IOException {
+ Configuration hadoopConf = new Configuration();
+ FileSystem fs = FileSystem.get(hadoopConf);
+ return new MapFileIterator(fs, mapFileName, hadoopConf);
+ }
+
+ @Override
+ public IteratorScope getIteratorScope() {
+ return scope.orElseThrow();
+ }
+
+ @Override
+ public boolean isFullMajorCompaction() {
+ if (getIteratorScope() != IteratorScope.majc) {
+ throw new IllegalStateException("Iterator scope is not majc");
+ }
+ return isFullMajorCompaction;
+ }
+
+ @Override
+ @Deprecated(since = "2.0.0")
+ public void registerSideChannel(SortedKeyValueIterator<Key,Value> iter) {
+ throw UOE;
+ }
+
+ @Override
+ public Authorizations getAuthorizations() {
+ if (getIteratorScope() != IteratorScope.scan) {
+ throw new IllegalStateException("Iterator scope is not majc");
+ }
+ return auths.orElseThrow();
+ }
+
+ @Override
+ public IteratorEnvironment cloneWithSamplingEnabled() {
+ if (!isSamplingEnabled()) {
+ throw new SampleNotPresentException();
+ }
+ return new ClientIteratorEnvironment(this);
+ }
+
+ @Override
+ public boolean isSamplingEnabled() {
+ return this.samplerConfig.isPresent();
+ }
+
+ @Override
+ public SamplerConfiguration getSamplerConfiguration() {
+ return samplerConfig.orElse(null);
+ }
+
+ @Override
+ public boolean isUserCompaction() {
+ if (getIteratorScope() == IteratorScope.scan) {
+ throw new IllegalStateException(
+ "scan iterator scope is incompatible with a possible user
compaction");
+ }
+ return this.isUserCompaction;
+ }
+
+ @Override
+ @Deprecated(since = "2.1.0")
+ public ServiceEnvironment getServiceEnv() {
+ return env.orElseThrow();
+ }
+
+ @Override
+ public PluginEnvironment getPluginEnv() {
+ return getServiceEnv();
Review Comment:
```suggestion
return env.orElseThrow();
```
Think it would be better to avoid calling the deprecated method for when
it's dropped
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -292,15 +280,54 @@ public Iterator<Entry<Key,Value>> iterator() {
EMPTY_BYTES, tableConf);
}
+ ClientServiceEnvironmentImpl senv = new
ClientServiceEnvironmentImpl(null) {
+
+ @Override
+ public String getTableName(TableId tableId) throws
TableNotFoundException {
+ throw new IllegalStateException("TableId not known in RFileScanner");
+ }
+
+ @Override
+ public <T> T instantiate(String className, Class<T> base)
+ throws ReflectiveOperationException, IOException {
+ return
RFileScanner.class.getClassLoader().loadClass(className).asSubclass(base)
+ .getDeclaredConstructor().newInstance();
+ }
+
+ @Override
+ public <T> T instantiate(TableId tableId, String className, Class<T>
base)
+ throws ReflectiveOperationException, IOException {
+ return instantiate(className, base);
+ }
+
+ @Override
+ public Configuration getConfiguration() {
+ return new ConfigurationImpl(new
ConfigurationCopy(DefaultConfiguration.getInstance()));
+ }
+
+ @Override
+ public Configuration getConfiguration(TableId tableId) {
+ return new ConfigurationImpl(new
ConfigurationCopy(opts.tableConfig));
+ }
+
+ };
+
+ ClientIteratorEnvironment.Builder iterEnvBuilder =
+ new RFileScannerIteratorEnvironmentBuilder().withEnvironment(senv)
+ .withAuthorizations(opts.auths).withScope(IteratorScope.scan);
+ if (getSamplerConfiguration() != null) {
+ iterEnvBuilder.withSamplerConfiguration(getSamplerConfiguration());
+ }
+ IteratorEnvironment iterEnv = iterEnvBuilder.build();
Review Comment:
I believe this needs a `withTableId`. While a table id doesn't make sense in
this case, I think the use case would be:
`env.getPluginEnv().getConfiguration(env.getTableId())`
See discussions: https://github.com/apache/accumulo/issues/4810 and
https://github.com/apache/accumulo/pull/4816/files#r1725159871 and
https://github.com/apache/accumulo/pull/4816#issuecomment-2347164444 (2nd
section)
##########
core/src/main/java/org/apache/accumulo/core/iterators/ClientIteratorEnvironment.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.iterators;
+
+import java.io.IOException;
+import java.util.Optional;
+
+import org.apache.accumulo.core.client.AccumuloClient;
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.client.SampleNotPresentException;
+import org.apache.accumulo.core.client.sample.SamplerConfiguration;
+import org.apache.accumulo.core.clientImpl.ClientContext;
+import org.apache.accumulo.core.clientImpl.ClientServiceEnvironmentImpl;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope;
+import org.apache.accumulo.core.iteratorsImpl.system.MapFileIterator;
+import org.apache.accumulo.core.security.Authorizations;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+
+public class ClientIteratorEnvironment implements IteratorEnvironment {
+
+ public static class Builder {
+
+ private Optional<IteratorScope> scope = Optional.empty();
+ private boolean isFullMajorCompaction = false;
+ private Optional<Authorizations> auths = Optional.empty();
+ private boolean isUserCompaction = false;
+ private Optional<TableId> tableId = Optional.empty();
+ private Optional<SamplerConfiguration> samplerConfig = Optional.empty();
+ protected Optional<ClientServiceEnvironmentImpl> env = Optional.empty();
+
+ public Builder withScope(IteratorScope scope) {
+ this.scope = Optional.of(scope);
+ return this;
+ }
+
+ public Builder isFullMajorCompaction() {
+ this.isFullMajorCompaction = true;
+ return this;
+ }
+
+ public Builder withAuthorizations(Authorizations auths) {
+ this.auths = Optional.of(auths);
+ return this;
+ }
+
+ public Builder isUserCompaction() {
+ this.isUserCompaction = true;
+ return this;
+ }
+
+ public Builder withTableId(TableId tableId) {
+ this.tableId = Optional.of(tableId);
+ return this;
+ }
+
+ public Builder withSamplerConfiguration(SamplerConfiguration sc) {
+ this.samplerConfig = Optional.of(sc);
+ return this;
+ }
+
+ public Builder withClient(AccumuloClient client) {
+ this.env = Optional.of(new ClientServiceEnvironmentImpl((ClientContext)
client));
+ return this;
+ }
Review Comment:
Could this be changed to take ClientContext to avoid casting? Seems like
what is passed is already always ClientContext
##########
core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java:
##########
@@ -295,9 +230,10 @@ public Iterator<Entry<Key,Value>> iterator() {
SortedKeyValueIterator<Key,Value> skvi;
try {
- IteratorEnvironment iterEnv = new ClientSideIteratorEnvironment(
- getSamplerConfiguration() != null,
getIteratorSamplerConfigurationInternal());
-
+ IteratorEnvironment iterEnv = new ClientIteratorEnvironment.Builder()
+ .withClient(context.get()).withAuthorizations(getAuthorizations())
+ .withScope(IteratorScope.scan).withTableId(tableId.get())
+
.withSamplerConfiguration(getIteratorSamplerConfigurationInternal()).build();
Review Comment:
Just want to confirm functionality is same before and after. Originally, was
passing `getSamplerConfiguration() != null` which would be the return value of
`isSamplingEnabled()`. Now, `isSamplingEnabled()` will always return true
(since the `samplerConfig` will always be present with value
`getIteratorSamplerConfigurationInternal()`)
Essentially, previously was using both `getSamplerConfiguration()` and
`getIteratorSamplerConfigurationInternal()` but is now just using
`getIteratorSamplerConfigurationInternal()`
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -292,15 +280,54 @@ public Iterator<Entry<Key,Value>> iterator() {
EMPTY_BYTES, tableConf);
}
+ ClientServiceEnvironmentImpl senv = new
ClientServiceEnvironmentImpl(null) {
+
+ @Override
+ public String getTableName(TableId tableId) throws
TableNotFoundException {
+ throw new IllegalStateException("TableId not known in RFileScanner");
+ }
+
+ @Override
+ public <T> T instantiate(String className, Class<T> base)
+ throws ReflectiveOperationException, IOException {
+ return
RFileScanner.class.getClassLoader().loadClass(className).asSubclass(base)
+ .getDeclaredConstructor().newInstance();
+ }
+
+ @Override
+ public <T> T instantiate(TableId tableId, String className, Class<T>
base)
+ throws ReflectiveOperationException, IOException {
+ return instantiate(className, base);
+ }
+
+ @Override
+ public Configuration getConfiguration() {
+ return new ConfigurationImpl(new
ConfigurationCopy(DefaultConfiguration.getInstance()));
+ }
+
+ @Override
+ public Configuration getConfiguration(TableId tableId) {
+ return new ConfigurationImpl(new
ConfigurationCopy(opts.tableConfig));
Review Comment:
When the `withTableId` is added, could change this to ensure the tableId is
the same:
```
Preconditions.checkArgument(tableId == getTableId(),
"Expected tableId obtained from
IteratorEnvironment.getTableId() but got " + tableId
+ " when requesting the table config");
```
(copied from #4816)
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/OfflineIterator.java:
##########
@@ -345,9 +245,13 @@ private SortedKeyValueIterator<Key,Value>
createIterator(KeyExtent extent,
MultiIterator multiIter = new MultiIterator(readers, extent);
- OfflineIteratorEnvironment iterEnv =
- new OfflineIteratorEnvironment(context, tableId, authorizations,
tableCC, false,
- samplerConfImpl == null ? null :
samplerConfImpl.toSamplerConfiguration());
+ ClientIteratorEnvironment.Builder iterEnvBuilder =
+ new
ClientIteratorEnvironment.Builder().withAuthorizations(authorizations)
+
.withScope(IteratorScope.scan).withTableId(tableId).withClient(context);
+ if (scannerSamplerConfig != null) {
+ iterEnvBuilder.withSamplerConfiguration(scannerSamplerConfig);
+ }
+ IteratorEnvironment iterEnv = iterEnvBuilder.build();
Review Comment:
It also seems like `isSamplingEnabled()` is always false in original impl,
but not in this impl
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
