HIVE-19880 : Repl Load to return recoverable vs non-recoverable error… #374
Conversation
| //method in HiveMetaStoreClient | ||
| REPL_EVENTS_MISSING_IN_METASTORE(20017, "Notification events are missing in the meta store."), | ||
|
|
||
| REPL_SOURCE_PATH_NOT_VALID(20018, "Target database is bootstrapped from some other path."), |
There was a problem hiding this comment.
Conflicting error name and message. Source path usually refers to source data path not dump path. Also, here the path is valid but couldn't perform bootstrap with it.
There was a problem hiding this comment.
changed to REPL_BOOTSTRAP_LOAD_PATH_NOT_VALID
| REPL_SOURCE_PATH_NOT_VALID(20018, "Target database is bootstrapped from some other path."), | ||
| REPL_FILE_MISSING_FROM_SRC_AND_CM_PATH(20019, "File is missing from both source and cm path."), | ||
| REPL_LOAD_PATH_NOT_FOUND(20020, "Load path does not exist."), | ||
| REPL_DATABASE_IS_NOT_SOURCE_OF_REPLICATION(20021, "Source of replication is not set in the database properties."), |
There was a problem hiding this comment.
Shall put the property key(repl.source.for) in the error message.
There was a problem hiding this comment.
source of replication is not set means either empty to null ..so can not put the value
| SPARK_GET_JOB_INFO_INTERRUPTED(30045, "Spark job was interrupted while getting job info"), | ||
| SPARK_GET_JOB_INFO_EXECUTIONERROR(30046, "Spark job failed in execution while getting job info due to exception {0}"), | ||
|
|
||
| REPL_FILE_SYSTEM_RETRY(30047, "Replication file system retry expired."), |
There was a problem hiding this comment.
REPL_FILE_SYSTEM_RETRY is not clear. What do you mean by file system retry?
it must REPL_DATA_COPY_RETRY_ERROR. Also, the message says "retry expired" which also sounds odd.
There was a problem hiding this comment.
can change to file system operation ..as it can be read or write
| LOG.error(StringUtils.stringifyException(e)); | ||
| setException(e); | ||
| return (1); | ||
| return ErrorMsg.getErrorMsg(e.getMessage()).getErrorCode(); |
There was a problem hiding this comment.
There are few return statements inside try block without throwing exception. Do we need to return error code there as well?
There was a problem hiding this comment.
currently only very few scenarios are covered ..in the followup jira need to add all other condition
| if (ReplChangeManager.isSourceOfReplication(database)) { | ||
| LOG.error("Cannot load database " + dbName + | ||
| " as it is a source of replication"); | ||
| throw new SemanticException(ErrorMsg.REPL_TARGET_IS_THE_SOURCE_OF_REPLICATION.getMsg()); |
There was a problem hiding this comment.
Why do we need to fail for this case? Isn't it enough to ensure, we don't overwrite this property when we load?
There was a problem hiding this comment.
when its already a source we should not load to it
There was a problem hiding this comment.
remove this error code
| if (!pathList.isEmpty()) { | ||
| LOG.error("File copy failed even after several attempts. Files list: " + pathList); | ||
| throw new IOException("File copy failed even after several attempts."); | ||
| throw new IOException(ErrorMsg.REPL_FILE_SYSTEM_RETRY.getMsg()); |
There was a problem hiding this comment.
This is File copy error not file system retry case.
There was a problem hiding this comment.
will be changed to file system operation error
| + "Also, bootstrap the system again to get back the consistent replicated state.", | ||
| nextEventId, e.getEventId()); | ||
| throw new IllegalStateException("Notification events are missing."); | ||
| throw new IllegalStateException(REPL_EVENTS_MISSING_IN_METASTORE); |
There was a problem hiding this comment.
Even this is non-recoverable error. So, shouldn't this be FatalException?
| private final ClientCapabilities version; | ||
|
|
||
| //copied from ErrorMsg.java | ||
| private static final String REPL_EVENTS_MISSING_IN_METASTORE = "Notification events are missing in the meta store."; |
There was a problem hiding this comment.
Why do we need this msg same as from ErrorMsg?
There was a problem hiding this comment.
as error code is hashed with error message
… codes : review comment fix
… codes : review comment fix2
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
2 participants
…