Sync data from S3 directly in GraphImporterTask #355
Conversation
fridex
force-pushed
the
gremlin-sync
branch
2 times, most recently
from
7037153 to
a46ab40
Compare
|
[test] |
fridex
force-pushed
the
gremlin-sync
branch
from
a46ab40 to
5cc7690
Compare
f8a_worker/workers/graph_importer.py
Outdated
| for task_name in tasks_to_sync: | ||
| if version_level: | ||
| task_result = s3.retrieve_task_result(task_name=task_name, **adapter_kwargs) | ||
| gremlin.store_task_result(task_name=task_name, task_result=task_result, **adapter_kwargs) |
There was a problem hiding this comment.
what about the if not version_level case ?
There was a problem hiding this comment.
Nice catch! The if-clause shouldn't be there.
fridex
force-pushed
the
gremlin-sync
branch
from
5cc7690 to
797bff1
Compare
|
@fridex - I guess lot of this code is already covered in #335. Do you want to add other files especially |
This should handle logic from task execution side. You can directly use this PR (changes from maintainers are allowed). Feel free to add missing files and logic. |
| object_key = self._construct_base_file_name(arguments) | ||
| bucket = self._s3.Bucket(self.bucket_name) | ||
| objects = bucket.objects.filter(Prefix=object_key) | ||
| return list(x.key for x in objects if x.key.endswith('.json')) |
There was a problem hiding this comment.
@miteshvp This will probably list task results with the whole object key ("path"). If you don't want to do it that way, feel free to adjust to return only task name.
EDIT: it will be probably needed anyway as this is handled by adapter.
@fridex in that case, WDYT of moving graph_importer version of this PR over to #335 rather than pulling all the other code here. Because apart from that, the adapter functionality is almost the same. Maybe you can give it another look there. |
I wanted to provide a template for better fit to core. If you are fine with maintaining this code, feel free to proceed with merge (please rebase in that case). |
|
@fridex - can you rebase this. LGTM, so that I can start working from where you left. |
I wouldn't merge this as this is a partial change. If we decide to deploy a new worker (e.g. due to intent API) and this change will be in, we will not sync anything to graph - changes that go in should be complete. I can rebase (heading to office). BTW you can still collaborate and push/rebase your changes. Changes by collaborators are enabled on github. |
We will not modify the flows currently. So we should be safe. Once we test this thoroughly then we can change the flow. WDYT? |
There is no modification of flows but there is amended GraphImporterTask itself that does not sync to graph in this PR rather it prints when/what is to be synced - only the sync part is missing - see https://github.com/fabric8-analytics/fabric8-analytics-worker/pull/355/files#diff-e7dcb5077b1c810edbe3839d1708dd73R14 and https://github.com/fabric8-analytics/fabric8-analytics-worker/pull/355/files#diff-e7dcb5077b1c810edbe3839d1708dd73R21 So if we merge this, there will not be any sync done. |
|
@miteshvp I did rebase for you, feel free to continue in this PR by pushing commits. |
|
@fridex thanks. I will work on this branch. Push the commits here directly. |
No description provided.