loaddata by alluxio engine #542
Conversation
XIAO-HOU
force-pushed
the
use_engine_loaddata
branch
from
a91a9a5
to
3949bf0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #542 +/- ##
==========================================
- Coverage 19.24% 19.05% -0.19%
==========================================
Files 60 62 +2
Lines 2411 2503 +92
==========================================
+ Hits 464 477 +13
- Misses 1898 1973 +75
- Partials 49 53 +4 ☔ View full report in Codecov by Sentry. |
XIAO-HOU
force-pushed
the
use_engine_loaddata
branch
from
187ba97
to
d29eaa5
Compare
XIAO-HOU
force-pushed
the
use_engine_loaddata
branch
from
3824e39
to
3ae491b
Compare
| return utils.RequeueIfError(err) | ||
| } | ||
| releaseName := utils.GetDataLoadReleaseName(targetDataload.Name) |
There was a problem hiding this comment.
如果只判断release是否安装成功，而不查看执行load的pod是否处于running或者pending，实际上不能反映当前的dataload操作是running还是pending。
There was a problem hiding this comment.
如果只判断release是否安装成功，而不查看执行load的pod是否处于running或者pending，实际上不能反映当前的dataload操作是running还是pending。
这里实际只是检查了release是否安装成功，具体判断当前的dataload操作是running还是pending实际是靠检查load job的状态，目前dataload是根据job的状态进入相应状态的。
There was a problem hiding this comment.
代码改动已完成，将返回Job状态的逻辑包含在了engine的loaddata()函数之内@cheyang
| // 1. Check if the helm release already exists | ||
| releaseName := utils.GetDataLoadReleaseName(ctx.Name) | ||
| jobName := utils.GetDataLoadJobName(releaseName) |
There was a problem hiding this comment.
通过获取helm release名再得到Job名的方式是否合理？我们是否应该允许用户通过helm之外的方式运行此job?
| // 1. Delete release if exists | ||
| releaseName := utils.GetDataLoadReleaseName(ctx.DataLoad.Name) | ||
| err := helm.DeleteReleaseIfExists(releaseName, ctx.DataLoad.Namespace) | ||
| releaseName := utils.GetDataLoadReleaseName(targetDataload.Name) | ||
| err := helm.DeleteReleaseIfExists(releaseName, targetDataload.Namespace) | ||
| if err != nil { | ||
| log.Error(err, "can't delete release", "releaseName", releaseName) | ||
| return utils.RequeueIfError(err) | ||
| } |
There was a problem hiding this comment.
我觉得不仅仅是创建job部分的逻辑，删除job部分的逻辑同样应该放到engine中。
目前的代码逻辑实际限制了启动load job只能通过helm方式。其他runtime开发者可能需要以其它方式运行此load job。
| if err = t.Implement.CreateDataLoadJob(ctx, targetDataload); err != nil { | ||
| t.Log.Error(err, "Failed to create the DataLoad job.") | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
这里由于我们alluxioruntime通过helm方式启动了此job，job名统一定为{$dataload}-loader-job，所以CreateDataLoadJob并未返回创建的job名。
后续直接通过此命名约定获取了此job名。
如果使用此逻辑，需要在文档中告诉runtime开发者，创建的loaderjob需要遵循此命名规范。
Ⅰ. Describe what this PR does
use engine to loaddata
Ⅱ. Does this pull request fix one issue?
fixes #295
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews