[GOBBLIN-204] Add a service that fetches GaaS flow configs from a git… #2055
Conversation
|
@abti, please review. |
| @@ -258,7 +258,7 @@ protected Spec readSpecFromFile(Path path) throws IOException { | |||
| Spec spec = null; | |||
|
|
|||
| try (FSDataInputStream fis = fs.open(path);) { | |||
| spec = this.specSerDe.deserialize(IOUtils.toByteArray(fis)); | |||
| spec = this.specSerDe.deserialize(ByteStreams.toByteArray(fis)); | |||
There was a problem hiding this comment.
why choose ByteStreams over IOUtils?
There was a problem hiding this comment.
This is due to toByteArray() being removed in v1.4 of apache-commons-codec, so I switched to the equivalent Guava method to avoid issues in the IDE with method not found errors
| void processGitConfigChanges() throws GitAPIException, IOException { | ||
| // if not active or if the flow catalog is not up yet then can't process config changes | ||
| if (!isActive || !this.flowCatalog.isRunning()) { | ||
| return; |
There was a problem hiding this comment.
nit: An info log might be helpful in debug?
| switch (change.getChangeType()) { | ||
| case ADD: | ||
| case MODIFY: | ||
| addSpec(change); |
There was a problem hiding this comment.
shouldn't MODIFY be delete and create?
There was a problem hiding this comment.
MODIFY is the update case. The FlowCatalog handles update in the put() call like add.
if (specStore.exists(spec.getUri())) {
specStore.updateSpec(spec);
this.listeners.onUpdateSpec(spec);
} else {
| // if the repository was not found then clone a new one | ||
| this.git = Git.cloneRepository() | ||
| .setDirectory(repoDirFile) | ||
| .setURI(this.repoUri) |
There was a problem hiding this comment.
Added config for setting branch.
| latestGitHash = head.getName(); | ||
|
|
||
| // diff old and new heads to find changes | ||
| ObjectReader reader = this.git.getRepository().newObjectReader(); |
There was a problem hiding this comment.
what happens if there is a conflict?
if someone force pushes, can this run into a detached head state?
There was a problem hiding this comment.
No changes are made locally, so there should be no conflicts other than issues due to forced pushes going back in time.
I changed the code to do a hard reset instead of pull to avoid any issues.
| private void addSpec(DiffEntry change) throws IOException { | ||
| if (checkConfigFilePath(change.getNewPath())) { | ||
| Path configFilePath = new Path(this.repositoryDir, change.getNewPath()); | ||
| Config flowConfig = loadConfigFileWithFlowNameOverrides(configFilePath); |
There was a problem hiding this comment.
Should we handle exception here if the config file is malformed and simply log the error instead of bubbling up?
There was a problem hiding this comment.
Added warning message.
|
LGTM, but the Travis is failing for this test: org.apache.gobblin.service.modules.core.GitConfigMonitorTest.cleanUp FAILED |
…is unreliable on Travis.
Closes apache#2055 from htran1/git_config
… repository
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
Add a service that fetches GaaS flow configs from a git repository.
Tests
Commits