-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat(controller): build dataset in server #2497
feat(controller): build dataset in server #2497
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2497 +/- ##
============================================
- Coverage 91.24% 83.01% -8.23%
- Complexity 0 2646 +2646
============================================
Files 102 448 +346
Lines 11785 24326 +12541
Branches 0 1452 +1452
============================================
+ Hits 10753 20194 +9441
- Misses 1032 3476 +2444
- Partials 0 656 +656
Flags with carried forward coverage won't be shown. Click here to find out more.
|
39349db
to
d359e70
Compare
2cc9f98
to
f8cb6f4
Compare
bd1f1d4
to
73cbce7
Compare
maybe we could reuse the job-task framework, so that there is no bother to write log collector/status watcher and token validator |
Yes, but this may be need extract some common logics, like whether to sync the job to datastore\ which resourcePool to use(image and dataset may use private resource) ..etc. so i will use annother PR(#2509) to fix them. |
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.
LGTM
var buildings = buildRecordMapper.selectBuildingInOneProjectForUpdate( | ||
project.getId(), request.getDatasetName()); |
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 line means we can apply two buildings with case-sensitive dataset names like mnist
and Mnist
?
And it seems that we do not allow this in Line 440:
if (!ds.getDatasetName().equalsIgnoreCase(request.getDatasetName())) {
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, you are right, i will fix it
var res = buildRecordMapper.insert(entity) > 0; | ||
if (res) { |
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.
Is it necessary to check the return value?
var dataset = datasetMapper.findByName(record.getDatasetName(), record.getProjectId(), false); | ||
if (dataset != null) { | ||
var version = datasetVersionMapper.findByLatest(dataset.getId()); | ||
if (version != null) { | ||
datasetVersionMapper.updateShared(version.getId(), true); | ||
} | ||
} |
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.
Make an issue to fix the race condition when the other user updates the dataset?
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.
ok, #2523
if (dataRootPath.endsWith("/")) { | ||
this.dataRootPath = dataRootPath; | ||
} else { | ||
this.dataRootPath = dataRootPath + "/"; | ||
} |
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.
Check if the root path is short enough to make sure that the generatePathPrefix
less than 255?
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.
ok
Description
Modules
Checklist