-
Notifications
You must be signed in to change notification settings - Fork 3
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
decrease memory utilization #50
Conversation
I will measure memory utilization and upload results later 🙏 |
} | ||
} | ||
} while (startDateTime != null && !startDateTime.isEmpty()); | ||
lastRow.getChangeEvent().getChangeDateTime(); |
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 code seems unused.
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.
fixed: de40057
|
||
response.iteratePages().iterator().forEachRemaining(pages::add); | ||
public void search(Consumer<Iterable<GoogleAdsServiceClient.SearchPage>> consumer, Map<String, String> params) { |
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.
nits: Although Iterator is called twice to get the last row, it is called once when the search method params change to per page.
public void search(Consumer<GoogleAdsServiceClient.SearchPage> consumer, Map<String, String> params) {
Iterable<GoogleAdsServiceClient.SearchPage> pages = search(params);
for (GoogleAdsServiceClient.SearchPage page : pages) {
consumer.accept(page);
lastPage = page;
}
if (!task.getResourceType().equals("change_event") || lastPage == null) {
return;
}
...
lastRow.getChangeEvent().getChangeDateTime(); | ||
Map<String, String> nextParams = new HashMap<>(); | ||
nextParams.put("start_datetime", lastRow.getChangeEvent().getChangeDateTime()); | ||
search(consumer, nextParams); |
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.
There is a risk that pages may not be released.
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.
fixed: 003f153
I fixed to pass single SearchPage
to consumer, and pages
is removed.
@chikamura I wrote about memory usage in the p-roverview, and fixed code. |
} | ||
pageBuilder.flush(); | ||
} | ||
Map<String, String> params = new HashMap<>(); |
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.
NOTE:
Since params is empty, the initial value of start_datetime is null.
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
description
to avoid oom error such as following.
it caused by iterate all pages and read result to memory. (implemented by #45)
embulk-input-google_ads/src/main/java/org/embulk/input/google_ads/GoogleAdsReporter.java
Line 72 in ababe29
running log
embulk config
Approximately 80,000 records can be retrieved under these config.yml in my env.
The following are the
jstat
log of the run with-J-Xmx400m
options.jstat(0.1.26)
When
S0
,E
, andO
are reached 100%, OOM occured.jstat(this branch)