[REEF-1978] Adding Checkpoint handler for IMRU master #1429

jwang98052 · 2018-02-02T20:34:43Z

Adding IMRUCheckpointHandler to handle task state persistent
Added configuration module for IMRUCheckpointHandler
Update IMRUJobDefination to all client to set checkpoint configuration
Add UpdateTaskStateCodec implementation
Update IMRU examples to set checkpoint config and call the check point handler
Update test cases

JIRA: REEF-1978
This closes #

* Adding IMRUCheckpointHandler to handle task state persistent * Added configuration module for IMRUCheckpointHandler * Update IMRUJobDefination to all client to set checkpoint configuration * Add UpdateTaskStateCodec implementation * Update IMRU examples to set checkpoint config and call the check point handler * Update test cases JIRA: [REEF-1978](https://issues.apache.org/jira/browse/REEF-1978) This closes #

markusweimer

I did a surface level read:

Please document all public classes, interfaces and their public members.
Make all new public classes sealed.
Check constructor parameters in the constructor instead of in methods to facilitate early failure and concise code.
Reformat all log lines not to contain #### and such. Also, consider moving them to higher log levels.
Reduce the number of new public classes and interfaces where possible.

markusweimer · 2018-03-12T15:49:43Z

lang/cs/Org.Apache.REEF.IMRU.Examples/PipelinedBroadcastReduce/PipelinedBroadcastAndReduce.cs

@@ -174,5 +176,24 @@ protected virtual IConfiguration BuildMapperFunctionConfig()
                    GenericType<BroadcastReceiverReduceSenderMapFunction>.Class)
                .Build();
        }
+
+        /// <summary>
+        /// Build checkpoint configuration. Subclass can override it.


I'd prefer not to use public abstract classes as APIs. Consider re-structuring this using composition instead of inheritance.

This is to follow the existing pattern for getting configurations. It was to let client to share the same CreateJobDefinitionBuilder but have its own way to override the configuration. If we really want to change it, it needs to do in different PR as the change must be consistent cross other methods.

markusweimer · 2018-03-12T15:50:01Z

....REEF.IMRU.Examples/PipelinedBroadcastReduce/PipelinedBroadcastAndReduceWithFaultTolerant.cs

+        /// <summary>
+        /// Build checkpoint configuration. Subclass can override it.
+        /// </summary>
+        protected override IConfiguration BuildCheckpointConfig()


Same as above.

Same as above. It is a sample client class, mainly contains driver configurations. I will change the class into internal.

markusweimer · 2018-03-12T15:50:26Z

....REEF.IMRU.Examples/PipelinedBroadcastReduce/PipelinedBroadcastAndReduceWithFaultTolerant.cs

+        /// </summary>
+        protected override IConfiguration BuildCheckpointConfig()
+        {
+            var filePath = Path.Combine(Path.GetTempPath(), Guid.NewGuid() + "state.txt");


Please use the temp files generated by REEF, not System.Path.

markusweimer · 2018-03-12T15:51:38Z

....REEF.IMRU.Examples/PipelinedBroadcastReduce/PipelinedBroadcastAndReduceWithFaultTolerant.cs

+
+            private void PersistState()
+            {
+                Logger.Log(Level.Info, "$$$$$$$$$$$ State to save: {0}", _taskState.Input[0]);


Please reformat all log lines and consider moving them to more fine grained log levels.

markusweimer · 2018-03-12T15:52:17Z

....REEF.IMRU.Examples/PipelinedBroadcastReduce/PipelinedBroadcastAndReduceWithFaultTolerant.cs

+            {
+                var obj = (UpdateTaskState<int[], int[]>)_stateHandler.Restore(_stateCodec);
+
+                if (obj != null)


How is obj used? Also, what if it is null?

obj is used to update the current state in momer
_taskState.Update(obj);

If null, that means the checkpoint handler is not able to get any old state for whatever reason, then the current state in the memory keeps the same.

markusweimer · 2018-03-12T16:09:04Z

lang/cs/Org.Apache.REEF.IMRU/OnREEF/CheckpointHandler/IMRUCheckpointHandler.cs

+        /// <returns></returns>
+        public ITaskState Restore(ICodec<ITaskState> codec)
+        {
+            if (!string.IsNullOrEmpty(_checkpointFilePath))


This should be validated in the constructor.

It is not for validation but for backward compatibility. If the client doesn't set it, we will do nothing but return null.

Yeah, it can be moved to constructor.

markusweimer · 2018-03-12T16:09:25Z

lang/cs/Org.Apache.REEF.IMRU/OnREEF/CheckpointHandler/IMRUCheckpointHandler.cs

+        {
+            if (!string.IsNullOrEmpty(_checkpointFilePath))
+            {
+                var files = _fileSystem.GetChildren(_fileSystem.CreateUriForPath(_checkpointFilePath));


The URI should have been created in the constructor.

Yes, it can be. Persist can be called many times and restore is called only once for a single recovery. So not much to optimize.

markusweimer · 2018-03-12T16:10:38Z

lang/cs/Org.Apache.REEF.IMRU/OnREEF/CheckpointHandler/IMRUCheckpointHandler.cs

+                        var localLatestFlagfile = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N").Substring(0, 4));
+                        var localLatestStatefile = Path.Combine(Path.GetTempPath() + Guid.NewGuid().ToString("N").Substring(0, 4));
+
+                        _fileSystem.CopyToLocal(latestFlagFile, localLatestFlagfile);


The copy to local should not be necessary. Can't you just .Open() the remote file?

Yeah, we can read directly from remote. I just want to match with the way the data is written to ensure the sate format.

markusweimer · 2018-03-12T16:12:49Z

lang/cs/Org.Apache.REEF.IMRU/OnREEF/CheckpointHandler/IMRUCheckpointResultHandler.cs

+        /// <returns></returns>
+        public bool GetResult()
+        {
+            if (!string.IsNullOrEmpty(_checkpointFilePath) && _fileSystem.Exists(_resultFileUrl))


The checks of readonly attributes should move to the constructor.

It is not really for validation but for backward compatibility. We want to make sure the code is still working if the client doesn't config it.

markusweimer · 2018-03-12T16:13:33Z

lang/cs/Org.Apache.REEF.IMRU/OnREEF/Driver/ConfigurationManager.cs

+            }
+            catch (Exception e)
+            {
+                Exceptions.Throw(e, "Unable to deserialize checkpoint configuration", Logger);


Remove the Exceptions use while you are at it :). Better yet, remove the useless catch here altogether.

Just to follow the existing pattern. Yes, the try catch is not necessary.

jwang98052 · 2018-03-14T19:20:16Z

@markusweimer I have addressed your review comments.

jwang98052 added 3 commits February 2, 2018 12:33

add save result portion

3e7df7a

persist mutilple states

91895c0

markusweimer suggested changes Mar 12, 2018

View reviewed changes

address review comments and handle failure case if state file corrupt

eeb766a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REEF-1978] Adding Checkpoint handler for IMRU master #1429

[REEF-1978] Adding Checkpoint handler for IMRU master #1429

jwang98052 commented Feb 2, 2018

markusweimer left a comment

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 14, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

markusweimer Mar 12, 2018

jwang98052 Mar 13, 2018

jwang98052 commented Mar 14, 2018

[REEF-1978] Adding Checkpoint handler for IMRU master #1429

Are you sure you want to change the base?

[REEF-1978] Adding Checkpoint handler for IMRU master #1429

Conversation

jwang98052 commented Feb 2, 2018

markusweimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwang98052 commented Mar 14, 2018