morningman commented on code in PR #47144:
URL: https://github.com/apache/doris/pull/47144#discussion_r1919530899
##########
fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java:
##########
@@ -184,24 +185,29 @@ public synchronized void loadIfNecessary(boolean force) {
|| currentTime - lastLoadTimeAuditLog >=
GlobalVariable.auditPluginMaxBatchInternalSec * 1000)) {
// begin to load
try {
- String token = "";
- try {
- // Acquire token from master
- token =
Env.getCurrentEnv().getTokenManager().acquireToken();
- } catch (Exception e) {
- LOG.warn("Failed to get auth token: {}", e);
- discardLogNum += auditLogNum;
- return;
- }
- AuditStreamLoader.LoadResponse response =
streamLoader.loadBatch(auditLogBuffer, token);
- if (LOG.isDebugEnabled()) {
- LOG.debug("audit loader response: {}", response);
- }
- } catch (Exception e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("encounter exception when putting current audit
batch, discard current batch", e);
+ while (true) {
Review Comment:
There are some issues:
1. `while(true)` is dangerous. If there is no more audit log coming and load
fail due to bug, this will run forever.
2. The cost of `exceedAuditDataSize()` is too high, it calls
`fillLogBuffer()` duplicately for each audit log, and there may be tens of
thousands of audit log in queue because you sleep 10 seconds after failure.
##########
fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java:
##########
@@ -184,24 +185,29 @@ public synchronized void loadIfNecessary(boolean force) {
|| currentTime - lastLoadTimeAuditLog >=
GlobalVariable.auditPluginMaxBatchInternalSec * 1000)) {
// begin to load
try {
- String token = "";
- try {
- // Acquire token from master
- token =
Env.getCurrentEnv().getTokenManager().acquireToken();
- } catch (Exception e) {
- LOG.warn("Failed to get auth token: {}", e);
- discardLogNum += auditLogNum;
- return;
- }
- AuditStreamLoader.LoadResponse response =
streamLoader.loadBatch(auditLogBuffer, token);
- if (LOG.isDebugEnabled()) {
- LOG.debug("audit loader response: {}", response);
- }
- } catch (Exception e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("encounter exception when putting current audit
batch, discard current batch", e);
+ while (true) {
Review Comment:
Also, we need add some UT to make sure this retry logic works well
##########
fe/fe-core/src/main/java/org/apache/doris/plugin/audit/AuditLoader.java:
##########
@@ -184,24 +185,29 @@ public synchronized void loadIfNecessary(boolean force) {
|| currentTime - lastLoadTimeAuditLog >=
GlobalVariable.auditPluginMaxBatchInternalSec * 1000)) {
// begin to load
try {
- String token = "";
- try {
- // Acquire token from master
- token =
Env.getCurrentEnv().getTokenManager().acquireToken();
- } catch (Exception e) {
- LOG.warn("Failed to get auth token: {}", e);
- discardLogNum += auditLogNum;
- return;
- }
- AuditStreamLoader.LoadResponse response =
streamLoader.loadBatch(auditLogBuffer, token);
- if (LOG.isDebugEnabled()) {
- LOG.debug("audit loader response: {}", response);
- }
- } catch (Exception e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("encounter exception when putting current audit
batch, discard current batch", e);
+ while (true) {
Review Comment:
I suggest:
1. At least add a `max_retry_time`
2. think twice about calling `fillLogBuffer()` in `exceedAuditDataSize ()`,
can we avoid that?
By the way, there is one more important thing about audit log,
which is how to make sure that the audit log in `fe.audit.log` file is
complete? Because despite of the audit table, we even can't guarantee the
integrity of `fe.audit.log`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
