#1698 OpenSearch: Make waitAck cache configurable via cache specifica…#1700
Conversation
…cification This commit replaces the hardcoded waitAck Caffeine cache configuration in StatusUpdaterBolt with a configurable cache specification string. It introduces the parameter `opensearch.status.waitack.cache.spec`, allowing users to control cache behavior (size, expiration) via config.
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left some comments.
| package org.apache.stormcrawler.opensearch.bolt; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
Please avoid star imports - also for the other occurences.
| } | ||
|
|
||
| @Test | ||
| @Timeout(value = 2, unit = TimeUnit.MINUTES) |
There was a problem hiding this comment.
Why is this test removed? If it fails, we should adjust it (or provide a reasoning why it was dropped)
There was a problem hiding this comment.
It was my bad.
I was trying to fix the Jacoco coverage issue and thought the test case was missing.
I’ll restore the original test case and include them in this commit.
There was a problem hiding this comment.
No problem. Thanks for your help on the issue! :)
| try { | ||
| bolt.prepare(conf, mockContext, mockCollector); | ||
| } catch (RuntimeException e) { | ||
| // 연결 실패 시 예외 분기 발생 → Jacoco branch coverage 확보 |
There was a problem hiding this comment.
Can we drop foreign language comments here please?
| assertTrue(e.getMessage().contains("Can't connect")); | ||
| } | ||
|
|
||
| Field field = StatusUpdaterBolt.class.getDeclaredField("waitAck"); |
There was a problem hiding this comment.
What are we trying to achieve here? Why do we check for the classname? If the class can't be initalized, the bolt is merely useless (so we should fail early), so no need for such a test imho.
There was a problem hiding this comment.
This test was only intended to verify that the value defined in the YAML config was correctly applied.
It was temporarily added while debugging a Jacoco coverage issue, so I’ll remove it and include the fix in this commit.
- update expireAfterWrite duration - avoid star imports - restore previous test code - remove Korean comments - delete unclear test case
jnioche
left a comment
jnioche
left a comment
There was a problem hiding this comment.
I would have done a bit differently
- load the value of topology.message.timeout.secs in statusupdaterbolt and use that as default.
- if opensearch.status.waitack.cache.spec is specified use it instead
- comment out the config in opensearch-conf.yaml and explain the logic
Bascially we'll use whatever is set as a message timeout unless specified otherwise
what do you think?
|
I agree that your suggested approach makes more sense. Also, I have one question: if "topology.message.timeout.secs" is not explicitly defined in "opensearch-conf.yaml", will calling it by key automatically get the default value (300s)? |
|
Yes. You have typically a cascade of YAML files when submitting a topology via flux for example, so it doesn't need to be defined in the OS specific config. |
|
@HyemIin thanks for your work on this BTW
|
3 participants
Thank you for contributing to Apache StormCrawler.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes
Is there a issue associated with this PR? Is it referenced in the commit message?
Does your PR title start with
#XXXXwhereXXXXis the issue number you are trying to resolve?Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
Is the code properly formatted with
mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?For code changes
mvn clean verify?Note
Please ensure that once the PR is submitted, you check GitHub Actions for build issues and submit an update to your PR as soon as possible.