-
Notifications
You must be signed in to change notification settings - Fork 877
ATLAS-4922: Atlas Async Import using Kafka #307
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
Conversation
a7dabe8
to
d678555
Compare
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.
@DishaTalreja3 - I haven't completed review yet; sending comments so far. Please review and update the patch.
atlas-examples/sample-app/src/main/java/org/apache/atlas/examples/sampleapp/SampleApp.java
Outdated
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Outdated
Show resolved
Hide resolved
new SynchronousQueue<>(), // Direct handoff queue | ||
new ThreadFactoryBuilder().setNameFormat(THREADNAME_PREFIX + " thread-%d").build()); | ||
} | ||
executors = executorService; |
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.
Review all writes to executors
i.e. following methods:
- startInternal()
- startConsumers()
- stop()
Prior to this patch, these methods are to be called only once in an Atlas instance. However, now executors
can be overwritten for every call to async import. Review the usage carefully and update to avoid inappropriate overwrites.
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.
In this new approach the executors are overWritten in
startConsumers() - only when the passed in executorService is null (essentially at the start of the instance or start of the first async import request). Please correct me f I am wrong.
8c56a08
to
5a3a995
Compare
5a3a995
to
69ecd1b
Compare
intg/src/main/java/org/apache/atlas/model/impexp/AtlasAsyncImportRequest.java
Outdated
Show resolved
Hide resolved
@JsonIgnoreProperties(ignoreUnknown = true) | ||
public class AtlasAsyncImportRequest extends AtlasBaseModelObject implements Serializable { | ||
private static final long serialVersionUID = 1L; | ||
public static final String ASYNC_IMPORT_TYPE_NAME = "__AtlasAsyncImportRequest"; |
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.
Constants defined in line 47-49 seem to be used/needed only in Atlas server side. If yes, please move these to server side (like repository module).
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.
These constants are used in this model class as well. Moving them to the repository module might introduce unnecessary dependencies.
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.
None of the classes in model package referenes ASYNC_IMPORT_TYPE_NAME
. This is referenced only from AsyncImportService
and AtlasAsyncImportRequestDTO
.
intg/src/main/java/org/apache/atlas/model/impexp/AtlasAsyncImportRequest.java
Outdated
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/model/notification/ImportNotification.java
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/model/notification/ImportNotification.java
Outdated
Show resolved
Hide resolved
KafkaProducer producer = getOrCreateProducer(topic); | ||
|
||
sendInternalToProducer(producer, topic, messages); | ||
sendInternal(topic, messages, SORT_NOT_NEEDED); |
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. I get it now. SORT_NOT_NEEDED
doesn't add much to readability, given the called method is right above. I suggest to simply send false
here.
notification/src/main/java/org/apache/atlas/notification/rest/RestNotification.java
Show resolved
Hide resolved
notification/src/main/java/org/apache/atlas/notification/spool/Spooler.java
Show resolved
Hide resolved
notification/src/test/java/org/apache/atlas/notification/spool/AtlasFileSpoolTest.java
Show resolved
Hide resolved
…mments and made improvements
7b2eb97
to
0f215e7
Compare
intg/src/main/java/org/apache/atlas/model/impexp/AsyncImportStatus.java
Outdated
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/model/impexp/AsyncImportStatus.java
Outdated
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/model/impexp/AsyncImportStatus.java
Outdated
Show resolved
Hide resolved
@JsonIgnoreProperties(ignoreUnknown = true) | ||
public class AtlasAsyncImportRequest extends AtlasBaseModelObject implements Serializable { | ||
private static final long serialVersionUID = 1L; | ||
public static final String ASYNC_IMPORT_TYPE_NAME = "__AtlasAsyncImportRequest"; |
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.
None of the classes in model package referenes ASYNC_IMPORT_TYPE_NAME
. This is referenced only from AsyncImportService
and AtlasAsyncImportRequestDTO
.
intg/src/main/java/org/apache/atlas/model/impexp/AtlasAsyncImportRequest.java
Outdated
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/model/notification/ImportNotification.java
Outdated
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/model/notification/ImportNotification.java
Outdated
Show resolved
Hide resolved
intg/src/main/java/org/apache/atlas/utils/AtlasAsyncImportTestUtil.java
Outdated
Show resolved
Hide resolved
intg/src/test/java/org/apache/atlas/utils/TestAtlasAsyncImportTestUtil.java
Outdated
Show resolved
Hide resolved
repository/src/main/java/org/apache/atlas/repository/impexp/AsyncImportService.java
Outdated
Show resolved
Hide resolved
repository/src/main/java/org/apache/atlas/repository/impexp/ImportService.java
Show resolved
Hide resolved
repository/src/main/java/org/apache/atlas/repository/impexp/ImportService.java
Show resolved
Hide resolved
...ory/src/main/java/org/apache/atlas/repository/store/graph/v2/bulkimport/MigrationImport.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Outdated
Show resolved
Hide resolved
af12b9f
to
099b166
Compare
099b166
to
83ed723
Compare
…ortTaskListenerImplTest
…ification of consumers and return 404 in case of import id not found
cf85053
to
594765e
Compare
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java
Outdated
Show resolved
Hide resolved
repository/src/main/java/org/apache/atlas/repository/impexp/ZipSource.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java
Show resolved
Hide resolved
webapp/src/main/java/org/apache/atlas/notification/ImportTaskListenerImpl.java
Outdated
Show resolved
Hide resolved
822868d
to
73d1ac1
Compare
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
client/client-v2/src/main/java/org/apache/atlas/AtlasClientV2.java
Outdated
Show resolved
Hide resolved
@DishaTalreja3 - can you please resolve conversations that you addressed? |
Co-authored-by: jackhalfalltrades <[email protected]> (cherry picked from commit f3d302a)
What changes were proposed in this pull request?
The existing synchronous import API causes performance bottlenecks with large or multiple requests. This patch introduces asynchronous imports using Kafka to queue requests via dedicated topics, enabling users to submit multiple import requests concurrently.
New APIs added to:
How was this patch tested?
Manual Testing, Unit Tests