[FLINK-1442] Reduce memory consumption of archived execution graph #344
Conversation
|
Looks good so far. I see that you removed the LRU code. Was that on purpose? Leaving it in may be a good idea, because the soft references are cleared in arbitrary order. It may make newer jobs disappear before older ones. Having the LRU in would mean things behave as previously as long as the memory is sufficient, and the soft reference clearing kicks in as a safety valve. |
|
@StephanEwen Yes, that was on purpose. The previous two data structures (
That's why |
| * @param jobID | ||
| * @return ExecutionGraph or null | ||
| */ | ||
| def getGraph(jobID: JobID) = graphs.get(jobID) match { |
There was a problem hiding this comment.
Why are we doing that? Why not working on the Option type? I don't like null.
|
I have actually merged this branch locally and already addressed most of the issues you found |
| val iter = graph.getAllExecutionVertices().iterator() | ||
| while (iter.hasNext) { | ||
| iter.next().clearExecutionEdges() | ||
| } |
There was a problem hiding this comment.
Why not using Scala power: graph.getAllExecutionVertices.asScala.foreach { _.clearExecutionEdges } with import scala.collection.JavaConvertes.iterableAsScalaIterableConverter in scope.
|
You can also shoot yourself in the foot with Option, no? There is no substitute for thorough programming... |
| private def cleanup(jobID: JobID): Unit = { | ||
| while (graphs.size > max_entries) { | ||
| // get first graph inserted | ||
| val (jobID, value) = graphs.iterator.next() |
There was a problem hiding this comment.
graphs.head equivalent to graphs.iterator.next() and shorter.
|
It is true that thorough programming is always the best, but Option encourages you to apply a more functional programming style which in many cases results in safer code. For example, Scala warns you if the pattern matching on Option is not exhaustive, whereas this is not the case for arbitrary classes and null values. |
|
Ah ok perfect that you already addressed the minor issues. |
…n graph This closes apache#344
…n graph This closes apache#344
…n graph This closes apache#344
…n graph This closes apache#344
|
Awesome! Thanks for the update. |
…n graph This closes apache#344
Co-authored-by: morsapaes <marta.paes.moreira@gmail.com> This closes apache#344.
No description provided.