tuxji commented on code in PR #886:
URL: https://github.com/apache/daffodil/pull/886#discussion_r1042763601
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala:
##########
@@ -224,14 +203,14 @@ class DataProcessor private (
}
// TODO Deprecate and replace usages with just tunables.
Review Comment:
I'd taken a look and backed away when I saw getTunable was used in
DFDL.DataProcessor, but I just looked again and I think it's feasible and
wouldn't require deprecation since getTunable doesn't seem to be part of the
Java or Scala API. The only places using getTunable seem to be some tests and
runtime1 itself. I'll replace getTunable with tunables.
```grep
./daffodil-core/src/test/scala/org/apache/daffodil/general/TestTunables.scala:66:
val t1 = dp1.getTunables()
./daffodil-core/src/test/scala/org/apache/daffodil/general/TestTunables.scala:67:
val t2 = dp2.getTunables()
./daffodil-core/src/test/scala/org/apache/daffodil/general/TestTunables.scala:72:
val t3 = dp2.getTunables() // modified tunables at 'run-time'
./daffodil-core/src/test/scala/org/apache/daffodil/general/TestTunables.scala:73:
val t4 = dp1.getTunables() // obtain first data processor to see if
anything changed
./daffodil-core/src/test/scala/org/apache/daffodil/infoset/TestInfosetCursor.scala:94:
infosetInputter.initialize(rootERD, u.getTunables())
./daffodil-core/src/test/scala/org/apache/daffodil/infoset/TestInfosetCursor1.scala:52:
ic.initialize(rootERD, u.getTunables())
./daffodil-core/src/test/scala/org/apache/daffodil/infoset/TestInfosetCursorFromReader.scala:58:
inputter.initialize(rootERD, u.getTunables())
./daffodil-core/src/test/scala/org/apache/daffodil/infoset/TestInfosetCursorFromReader2.scala:66:
inputter.initialize(rootERD, u.getTunables())
./daffodil-core/src/test/scala/org/apache/daffodil/util/TestUtils.scala:348:
override def getTunables(): DaffodilTunables = { tunables }
./daffodil-runtime1/src/main/scala/org/apache/daffodil/api/DFDLParserUnparser.scala:195:
def getTunables(): DaffodilTunables
./daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DaffodilUnparseContentHandler.scala:81:
private lazy val tunablesBatchSize = dp.getTunables().saxUnparseEventBatchSize
./daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala:206:
def getTunables(): DaffodilTunables = tunables
./daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala:476:
inputter.initialize(ssrd.elementRuntimeData, getTunables())
./daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala:661:
val tunables = dataProc.getTunables()
./daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala:691:
val tunables = dataProc.getTunables()
./daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala:692:
dataProc.getTunables(),
./daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/Runtime2DataProcessor.scala:66:
override def getTunables(): DaffodilTunables = ???
```
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala:
##########
@@ -311,18 +271,13 @@ class DataProcessor private (
val oos = new ObjectOutputStream(new GZIPOutputStream(os))
//
- // Make a copy of this object, so that our state mods below don't
side-effect the user's object.
- // Saving shouldn't have side-effects on the state of the object.
- //
- //
- // Note that the serialization system *does* preserve these two settings.
This is for general serialization
- // that may be required by other software (e.g., Apache Spark)
+ // Make a copy of this object so that we can make its saved state
+ // different than its original state. Note that the serialization
+ // system *does* preserve validationMode since it may be required by
+ // other software like Apache Spark. But for our save/reload purposes,
+ // we don't want validationMode preserved.
//
- // But for our save/reload purposes, we don't want them preserved.
- //
-
val dpToSave = this.copy(
- externalVars = Queue.empty[Binding], // explicitly set these to empty so
restored processor won't have them.
Review Comment:
I think you meant `variableMap = ssrd.originalVariables` since we've gotten
rid of all uses/mentions of `externalVars`. Anyway, should a saved processor
preserve its own `variableMap` or replace it with `ssrd.originalVariables`?
That is the real question we need to ask. Mike @mbeckerle , do you want to
weigh in?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala:
##########
@@ -84,27 +83,25 @@ class InvalidUsageException(msg: String, cause: Throwable =
null) extends Except
object DataProcessor {
/**
- * This is the DataProcessor constructed from a saved processor.
+ * This is the SerializableDataProcessor constructed from a saved processor.
*
* It enables us to implement restrictions on what you can/cannot do with a
reloaded
* processor versus an original one.
*
- * When we create one of these, it will have default values for everything
- * settable like debuggers, debug mode.
+ * When we reload a processor, it will have default values for everything
settable
+ * like validation mode, debug mode, and debugger.
*
- * Note that this does preserve the externalVars and validationMode. That is
because
- * those may be needed by serializations other than our own save/reload
(e.g., Apache Spark which
- * serializes to move things for remote execution).
+ * Note that this class does preserve validationMode. That is because
validationMode
+ * may be needed by serializations other than our own save/reload (e.g.,
Apache Spark
+ * which serializes to move objects for remote execution).
*
- * Hence, we're depending on the save method to explicitly reset
validationMode and
- * externalVars to initial values.
+ * Hence, we're depending on the save method to explicitly turn off
validationMode.
*/
private class SerializableDataProcessor(
val data: SchemaSetRuntimeData,
tunable: DaffodilTunables,
- externalVars: Queue[Binding], // must be explicitly set to empty by save
method
- validationModeArg: ValidationMode.Type) // must be explicitly set from
Full to Limited by save method.
- extends DataProcessor(data, tunable, externalVars, validationModeArg) {
+ validationModeArg: ValidationMode.Type) // must be explicitly turned off
by save method
+ extends DataProcessor(data, tunable, data.originalVariables,
validationModeArg) {
Review Comment:
The `data.originalVariable` is a method which returns a copy of the
VariableMap stored in `data` so that we can proceed to serialize the
SerializableDataProcessor even if another thread tries to mutate the
VariableMap stored in `data` at the same time. Are you suggesting that
SerializableDataProcessor should have 4 (instead of 3) arguments with
VariableMap becoming the 3rd parameter and validationModeArg becoming the 4th
parameter? All so that our writeReplace method can pass the DataProcessor's
own VariableMap to the SerializableDataProcessor constructor instead of ssrd's
(data's) VariableMap getting passed?
I think that makes sense since the withExternalVariables methods return a
copy of the DataProcessor with a new VariableMap and we should serialize that
new VariableMap instead of the original VariableMap we started with.
VariableMap says it's not thread-safe, so I'll make writeReplace pass
variableMap.copy() just to be as safe as possible.
--
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]
