IGNITE-24062 Use throttled logger for heap lock manager #5015
Conversation
| /** Throttle timeout in milliseconds (value is 5 min). */ | ||
| long THROTTLE_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(5); | ||
| /** JVM property to configure a throttle interval. */ | ||
| String THROTTLE_TIMEOUT_MILLIS = "IGNITE_THROTTLE_TIMEOUT_MILLIS"; |
There was a problem hiding this comment.
Should we provide any user documentation on the property?
There was a problem hiding this comment.
I added a release note for this ticket.
Currently, we do not have other options.
There was a problem hiding this comment.
Also after the slack discussion we consider all system properties as internal details and wouldn't make a documentation.
| String THROTTLE_TIMEOUT_MILLIS = "IGNITE_THROTTLE_TIMEOUT_MILLIS"; | ||
|
|
||
| /** Default throttle timeout in milliseconds (value is 5 min). */ | ||
| long DEFAULT_THROTTLE_TIMEOUT = TimeUnit.MINUTES.toMillis(5); |
There was a problem hiding this comment.
Also I would like to save MILLIS postfix for the default value and name the property name field:
static final- named as
THROTTLE_TIMEOUT_MILLIS_PROPERTY_NAME, it's more verbose, but there is distinct between prop's name and it's value.
There was a problem hiding this comment.
- It is an interface. So it has public static final modifiers by default.
- I renamed the variable and the property name also. I do not think it is necessary to use the *_PROPERTY_NAME prefix.
There was a problem hiding this comment.
- My bad.
- Up to you, but we have 2 variables
DEFAULT_LOG_THROTTLE_INTERVAL_MSandLOG_THROTTLE_INTERVAL_MSwhere the first one islongand the last one isString, their purposes and actual value are different, that's why I suggested to name a property name in this way.
But still, we have no now any conventions, so it's just my proposal.
| private static final IgniteLogger THROTTLED_LOG = Loggers.toThrottledLogger( | ||
| Loggers.forClass(HeapLockManager.class), | ||
| // TODO: IGNITE-24181 Get rid of Common thread pool. | ||
| ForkJoinPool.commonPool() |
There was a problem hiding this comment.
Why ForkJoinPool there? May we pass ThreadPoolsManager#commonScheduler there from IgniteImpl in time? I saw IGNITE-24181, but just thought that common scheduler may be more reasonable for now.
There was a problem hiding this comment.
I do not want to drag an executor here because it would be deleted in the mentioned ticket.
https://issues.apache.org/jira/browse/IGNITE-24062