Skip to content
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

Some bug fix for load task and schema manage #246

Merged
merged 7 commits into from
Oct 30, 2020
Merged

Some bug fix for load task and schema manage #246

merged 7 commits into from
Oct 30, 2020

Conversation

Linary
Copy link
Contributor

@Linary Linary commented Sep 29, 2020

No description provided.

@Linary Linary force-pushed the 1.5.0-bug-fix branch 3 times, most recently from 7e582ac to efe4ed9 Compare September 29, 2020 14:14
this.fileReadLines = this.context().newProgress().totalInputReaded();
this.lastDuration += this.context().summary().totalTime();
this.currDuration = 0L;
// how to save db
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove it

while (this.status.inRunning()) {
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to ignored

log.info("LoadTask is executing run task:{}", task.getId());
task.run();
log.info("Execute callback");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update log to:
log.info("Executing task: {}", task.getId());
log.info("Executed task: {}", task.getId());

@Linary Linary force-pushed the 1.5.0-bug-fix branch 2 times, most recently from 9c40dc3 to e794dd3 Compare October 16, 2020 09:58
try {
// transferTo should accept absolute path
srcFile.transferTo(destFile.getAbsoluteFile());
result.setStatus(FileUploadResult.Status.SUCCESS);
log.info("Uploaded file part {}", partName + "-" + index);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Uploaded file part {}-{}",

@@ -160,7 +168,8 @@ public boolean tryMergePartFiles(String dirPath, int total) {
// Rename file to dest file
FileUtils.moveFile(partFiles[0], newFile);
} catch (IOException e) {
throw new InternalException("load.upload.move-file.failed");
log.error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some message for log

@@ -178,19 +187,22 @@ public boolean tryMergePartFiles(String dirPath, int total) {
try (InputStream is = new FileInputStream(partFile)) {
IOUtils.copy(is, os);
} catch (IOException e) {
log.error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}
}
// Delete origin directory
try {
FileUtils.forceDelete(dir);
} catch (IOException e) {
throw new InternalException("load.upload.delete-temp-dir.failed");
log.error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Iterator<Task> tasks = list.iterator();
while (tasks.hasNext()) {
Task task = tasks.next();
for (Task task : list) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename list to tasks

LoadContext context = task.context();
task.setFileReadLines(context.newProgress().totalInputReaded());
task.setCurrDuration(context.summary().totalTime());
if (this.update(task) != 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does 1 mean, prefer boolean or enum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 means operate affected rows. it defined by mybatis-plus framework.

@@ -178,19 +188,23 @@ public boolean tryMergePartFiles(String dirPath, int total) {
try (InputStream is = new FileInputStream(partFile)) {
IOUtils.copy(is, os);
} catch (IOException e) {
log.error("Failed copy file stream from {} to {}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed to copy

}
}
} catch (IOException e) {
throw new InternalException("load.upload.merge-file.failed");
log.error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improve

throw new InternalException("entity.update.failed", task);
task.getOptions().incrementalMode = true;
task.restoreContext();
task.lock.lock();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to add method task.lock()/unlock()

public class LoadTask implements Runnable {

@TableField(exist = false)
@JsonIgnore
private transient HugeGraphLoader loader;
public transient final Lock lock = new ReentrantLock();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set private

@PathVariable("jobId") int jobId,
@RequestParam("name") String fileName) {
String token = this.service.generateFileToken(fileName);
this.uploadingTokenLocks.put(token, new ReentrantReadWriteLock());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check exist

Iterator<Map.Entry<Integer, LoadTask>> iter = taskContainer.entrySet()
.iterator();
Iterator<Map.Entry<Integer, LoadTask>> iter;
iter = this.taskContainer.entrySet().iterator();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runningTasks

for (String fileName : fileNames) {
String token = this.service.generateFileToken(fileName);
Ex.check(!this.uploadingTokenLocks.containsKey(token),
"load.upload.file.token.existed");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check put() return null

@Linary Linary force-pushed the 1.5.0-bug-fix branch 2 times, most recently from d67f793 to 670d887 Compare October 22, 2020 06:04
@@ -286,6 +291,17 @@ public void loadParameter(@RequestBody LoadParameter newEntity) {
}
}

@PutMapping("finish")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer rename to another name, like ready-loading

@@ -240,6 +247,17 @@ public Boolean delete(@PathVariable("connId") int connId,
}
}

@PutMapping("finish")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

new ConfigOption<>(
"upload_file.max_uploading_time",
"The maximum allowable uploading time(second) for file " +
"uploads, the uploaded parts will be cleared if exceed " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uploaded file parts

try {
FileUtils.forceDelete(new File(filePath));
} catch (IOException e) {
log.warn("Failed to delete uploading timeout file {}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete expired uploading files

throw new InternalException("entity.update.failed", task);
}
// executed in other threads
this.taskExecutor.execute(task, () -> this.update(task));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should handle exception?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should handle exception if the update is failed?

try {
if (this.update(task) != 1) {
throw new InternalException("entity.update.failed", task);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we wrap these 3 lines with a method

if (this.update(task) != 1) {
throw new InternalException("entity.update.failed", task);
task.getOptions().incrementalMode = true;
task.restoreContext();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move line 200~2201 to try block

@@ -99,7 +101,8 @@ public ExecuteHistory get(int connId, int id) {
try {
Task task = client.task().get(history.getAsyncId());
history.setDuration(task.updateTime() - task.createTime());
history.setAsyncStatus(AsyncTaskStatus.valueOf(task.status().toUpperCase()));
history.setAsyncStatus(AsyncTaskStatus.valueOf(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer add a method setAsyncStatus(string)

}
this.save(task);
// executed in other threads
this.taskExecutor.execute(task, () -> this.update(task));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should handle exception if the update is failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need, it doesn't affect other tasks to use the thread pool

javeme
javeme previously approved these changes Oct 28, 2020
@@ -130,18 +132,14 @@ public FileMapping fileSetting(@PathVariable("id") int id,
if (mapping == null) {
throw new ExternalException("load.file-mapping.not-exist.id", id);
}
// unescape \\t to \t
newEntity.unescapeDelimiterIfNeeded();
// change format to TEXT if needed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Change format to TEXT if needed


@JsonIgnore
public Set<String> getVertexMappingLabels() {
if (this.getVertexMappings() != null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (this.getVertexMappings() != null) {
    return new HashSet<>();
}
return this.getVertexMappings().stream()
           .map(ElementMapping::getLabel)
           .collect(Collectors.toSet());


@JsonIgnore
public Set<String> getEdgeMappingLabels() {
if (this.getEdgeMappings() != null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

this.status = LoadStatus.FAILED;
this.lock.lock();
try {
// Pay attention to whether the user stops actively or
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*
*/

float rate;
if (this.fileReadLines == null || this.duration == null ||
this.duration == 0L) {
if (readLines == 0L || duration == 0L) {
rate = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rate = 0.0F

@Linary Linary merged commit 872887b into master Oct 30, 2020
@Linary Linary deleted the 1.5.0-bug-fix branch October 30, 2020 05:08
OshotOkill pushed a commit to OshotOkill/hugegraph-hubble that referenced this pull request Nov 3, 2020
OshotOkill pushed a commit to OshotOkill/hugegraph-hubble that referenced this pull request Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants