New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-22954 Repl Load using scheduler #932
Conversation
67aeea1
to
54b5ea3
Compare
@@ -121,7 +121,9 @@ public String getName() { | |||
public int execute() { | |||
try { | |||
Hive hiveDb = getHive(); | |||
Path dumpRoot = new Path(conf.getVar(HiveConf.ConfVars.REPLDIR), work.dbNameOrPattern.toLowerCase()); | |||
Path dumpRoot = new Path(conf.getVar(HiveConf.ConfVars.REPLDIR), | |||
Base64.getEncoder().encodeToString(work.dbNameOrPattern.toLowerCase() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we are not going to have expression ( for ex for table level replication ) do we still need the base64 encoding ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will still need repl dump *
ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
for (FileStatus status : statuses) { | ||
DumpMetaData dmd = new DumpMetaData(status.getPath(), conf); | ||
if (dmd.isIncrementalDump() | ||
&& Long.parseLong(currentReplStatusOfTarget) < dmd.getEventTo().longValue()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.longValue() function is not required.
also why not do comparison with EventFrom will be easy to reason about in code. since the repl status should be either 1 less than from or equal to from event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are test cases where incremental event starts from say (3-15) and current status of target is at 10 because of the previous partial incremental load.
Like TestReplicationWithTableMigration#testIncrementalLoadMigrationManagedToAcidFailurePart
ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
String replLastId = getReplStatus(dbNameOrPattern); | ||
prepareReturnValues(Collections.singletonList(replLastId), "last_repl_id#string"); | ||
setFetchTask(createFetchTask("last_repl_id#string")); | ||
LOG.debug("ReplicationSemanticAnalyzer.analyzeReplStatus: writing repl.last.id={} out to {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to print the full conf here ? if yes you might want to provide a String representation of it along with an additional {} placeholder in the message to put the values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conf just prints the configuration files and not all the configs. This was part of older repl status code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having files printed might not be useful, it might be good to have the full configs printed in debug or may be trace mode additionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes printing conf in debug mode. The toString is already implemented in Configuration. I missed it.
ql/src/java/org/apache/hadoop/hive/ql/parse/ReplicationSemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
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. |
No description provided.