[ 
https://issues.apache.org/jira/browse/YARN-2288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14086535#comment-14086535
 ] 

Zhijie Shen commented on YARN-2288:
-----------------------------------

Thanks for the patch, [~djp]! Here're some comments:

1. Should we do the similar thing for MemoryTimelineStore? Though it is used to 
for testing and playing the feature, we never prevent users from choosing it, 
and it is subject to changes in the future as well.

2. Change to TIMELINE_STORE_VERSION_KEY = "timeline-store-version"? Should we 
move the constants to TimelineStore, such that it can be inherited by different 
store implementation. If CURRENT_VERSION_INFO may differ for different stores, 
it can be kept here.
{code}
+  private static final String TS_STORE_VERSION_KEY = "ts-store-version";
+  
+  private static final Version CURRENT_VERSION_INFO = Version
+      .newInstance(1, 0);
{code}

3. Log is not correct
{code}
+    LOG.info("Loaded NM state version info " + loadedVersion);
{code}

4. TS -> timeline? And the last comment about NM is not correct here.
{code}
+   * 1) Versioning TS store: major.minor. For e.g. 1.0, 1.1, 1.2...1.25, 2.0 
etc.
+   * 2) Any incompatible change of TS-store is a major upgrade, and any
+   *    compatible change of TS-store is a minor upgrade.
+   * 3) Within a minor upgrade, say 1.1 to 1.2:
+   *    overwrite the version info and proceed as normal.
+   * 4) Within a major upgrade, say 1.2 to 2.0:
+   *    throw exception and indicate user to use a separate upgrade tool to
+   *    upgrade NM state or remove incompatible old state.
+   */
{code}

5. The one-line wrapper method seems to be unnecessary.
{code}
+  private void storeVersion() throws IOException {
+    dbStoreVersion(CURRENT_VERSION_INFO);
+  }
+  
+  // Only used for test
+  @VisibleForTesting
+  void storeVersion(Version state) throws IOException {
+    dbStoreVersion(state);
+  }
{code}

6. Log an fatal message here? Though the caller(Service) seems to log it as 
well, but IMHO, we shouldn't rely on it for the critical message.
{code}
checkVersion
{code}

> Data persistent in timelinestore should be versioned
> ----------------------------------------------------
>
>                 Key: YARN-2288
>                 URL: https://issues.apache.org/jira/browse/YARN-2288
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: 2.4.1
>            Reporter: Junping Du
>            Assignee: Junping Du
>         Attachments: YARN-2288.patch
>
>
> We have LevelDB-backed TimelineStore, it should have schema version for 
> changes in schema in future.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to