keith-turner commented on code in PR #4816:
URL: https://github.com/apache/accumulo/pull/4816#discussion_r1725179686
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -335,6 +352,52 @@ public boolean isSamplingEnabled() {
public SamplerConfiguration getSamplerConfiguration() {
return RFileScanner.this.getSamplerConfiguration();
}
+
+ @Override
+ public TableId getTableId() {
+ return null;
+ }
+
+ @Override
+ @Deprecated(since = "2.0.0")
+ public AccumuloConfiguration getConfig() {
+ return tableConf;
+ }
+
+ @Override
+ @Deprecated(since = "2.1.0")
+ public ServiceEnvironment getServiceEnv() {
+ return serviceEnvironment.get();
+ }
+
+ private ServiceEnvironment createServiceEnv() {
+ return new ServiceEnvironment() {
+ @Override
+ public <T> T instantiate(TableId tableId, String className, Class<T>
base) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public <T> T instantiate(String className, Class<T> base) {
+ throw new UnsupportedOperationException();
Review Comment:
Could implement this in a similar way to
https://github.com/apache/accumulo/blob/fd111a22a6454837ad6076d5ba6fe54fc8934be5/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientServiceEnvironmentImpl.java#L65-L69
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -335,6 +352,52 @@ public boolean isSamplingEnabled() {
public SamplerConfiguration getSamplerConfiguration() {
return RFileScanner.this.getSamplerConfiguration();
}
+
+ @Override
+ public TableId getTableId() {
+ return null;
Review Comment:
Returning null could cause an NPE in calling code that could be much later
than this call and if the NPE is in a compound statement it could be hard to
work back to the cause. Thinking it would be better to fail fast rather than
return null.
```suggestion
throw new UnsupportedOperationException("This scanner is unrelated to
any table or accumulo instance, it is operating directly on files."):
```
Alternatively we could always return a dummy table id here.
```suggestion
return TableId.of("rfileScannerFakeD");
```
If we did that then the methods that take a table id could possibly only
accept this id.
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -335,6 +352,52 @@ public boolean isSamplingEnabled() {
public SamplerConfiguration getSamplerConfiguration() {
return RFileScanner.this.getSamplerConfiguration();
}
+
+ @Override
+ public TableId getTableId() {
+ return null;
+ }
+
+ @Override
+ @Deprecated(since = "2.0.0")
+ public AccumuloConfiguration getConfig() {
+ return tableConf;
+ }
+
+ @Override
+ @Deprecated(since = "2.1.0")
+ public ServiceEnvironment getServiceEnv() {
+ return serviceEnvironment.get();
+ }
+
+ private ServiceEnvironment createServiceEnv() {
+ return new ServiceEnvironment() {
+ @Override
+ public <T> T instantiate(TableId tableId, String className, Class<T>
base) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public <T> T instantiate(String className, Class<T> base) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String getTableName(TableId tableId) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public Configuration getConfiguration(TableId tableId) {
Review Comment:
It would be nice to update the javadoc for the following methods.
https://github.com/apache/accumulo/blob/fd111a22a6454837ad6076d5ba6fe54fc8934be5/core/src/main/java/org/apache/accumulo/core/client/rfile/RFile.java#L178-L187
To state that the iterator env will return the properties passed into these
methods. Could also include the version number in the docs. Like `Starting
with 2.1.4 <link to iter env method> will return the properties passed in here`
##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -335,6 +352,52 @@ public boolean isSamplingEnabled() {
public SamplerConfiguration getSamplerConfiguration() {
return RFileScanner.this.getSamplerConfiguration();
}
+
+ @Override
+ public TableId getTableId() {
+ return null;
+ }
+
+ @Override
+ @Deprecated(since = "2.0.0")
+ public AccumuloConfiguration getConfig() {
+ return tableConf;
+ }
+
+ @Override
+ @Deprecated(since = "2.1.0")
+ public ServiceEnvironment getServiceEnv() {
+ return serviceEnvironment.get();
+ }
+
+ private ServiceEnvironment createServiceEnv() {
+ return new ServiceEnvironment() {
+ @Override
+ public <T> T instantiate(TableId tableId, String className, Class<T>
base) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public <T> T instantiate(String className, Class<T> base) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String getTableName(TableId tableId) {
+ throw new UnsupportedOperationException();
Review Comment:
All of these exceptions could include the same message. Maybe make the
message a private constant.
```suggestion
throw new UnsupportedOperationException("This scanner is unrelated
to any table or accumulo instance, it is operating directly on files. Therefore
it can not support this operation.");
```
--
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]
