[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325439739 ## File path: pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java ## @@ -41,20 +43,25 @@ private static ControllerLeaderLocator _instance = null; public static final Logger LOGGER = LoggerFactory.getLogger(ControllerLeaderLocator.class); + // Minimum millis which must elapse between consecutive invalidation of cache + private static final long MILLIS_BETWEEN_INVALIDATE = 30_000L; + private final HelixManager _helixManager; - // Co-ordinates of the last known controller leader. - private Pair _controllerLeaderHostPort = null; + // Co-ordinates of the last known controller leader for each of the lead-controller every partitions, + // with partition number being the key and controller hostname and port pair being the value. If the lead + // controller resource is disabled in the configuration then this map contains helix cluster leader co-ordinates + // for all partitions of leadControllerResource. + private final Map> _cachedControllerLeaderMap; - // Indicates whether cached controller leader value is invalid - private volatile boolean _cachedControllerLeaderInvalid = true; + // Indicates whether cached controller leader(s) value is(are) invalid. + private volatile boolean _cachedControllerLeaderValid = false; // Time in millis when cache invalidate was last set private volatile long _lastCacheInvalidateMillis = 0; Review comment: `_lastCacheInvalidationTimeMs` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440855
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440772
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
Review comment:
(nit) This check can be merged into the next one
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325437581 ## File path: pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java ## @@ -41,20 +43,25 @@ private static ControllerLeaderLocator _instance = null; public static final Logger LOGGER = LoggerFactory.getLogger(ControllerLeaderLocator.class); + // Minimum millis which must elapse between consecutive invalidation of cache + private static final long MILLIS_BETWEEN_INVALIDATE = 30_000L; Review comment: Rename to `MIN_INVALIDATE_INTERVAL_MS`? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325439471
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -229,22 +251,26 @@ private boolean isLeadControllerResourceEnabled() {
* Thus the frequency limiting is done to guard against frequent cache
refreshes, in cases where we might be getting too many NOT_SENT responses due
to some other errors.
*/
public synchronized void invalidateCachedControllerLeader() {
-long now = System.currentTimeMillis();
+long now = getCurrentTimeMS();
long millisSinceLastInvalidate = now - _lastCacheInvalidateMillis;
if (millisSinceLastInvalidate < MILLIS_BETWEEN_INVALIDATE) {
LOGGER.info(
"Millis since last controller cache value invalidate {} is less than
allowed frequency {}. Skipping invalidate.",
millisSinceLastInvalidate, MILLIS_BETWEEN_INVALIDATE);
} else {
LOGGER.info("Invalidating cached controller leader value");
- _cachedControllerLeaderInvalid = true;
+ _cachedControllerLeaderValid = false;
_lastCacheInvalidateMillis = now;
}
}
+ protected long getCurrentTimeMS() {
Review comment:
Rename to `getCurrentTimeMs()`. Also, is this visible for testing?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325441081
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ Set partitionNames =
leadControllerResourceExternalView.getPartitionSet();
+ if (partitionNames.isEmpty()) {
+LOGGER.warn("The partition set in the external view of {} is empty.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+return;
+ }
+ if (partitionNames.size() !=
Helix.NUMBER_OF_PARTITIONS_IN_LEAD_CONTROLLER_RESOURCE) {
+LOGGER.warn("The partition size of
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440450
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
try {
- ZNRecord znRecord = dataAccessor.get("/" +
_helixManager.getClusterName() + "/CONFIGS/RESOURCE/"
- + CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_NAME, stat,
AccessOption.THROW_EXCEPTION_IFNOTEXIST);
- return
Boolean.parseBoolean(znRecord.getSimpleField(CommonConstants.Helix.LEAD_CONTROLLER_RESOURCE_ENABLED_KEY));
+ ExternalView leadControllerResourceExternalView =
_helixManager.getClusterManagmentTool()
+ .getResourceExternalView(_helixManager.getClusterName(),
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
+ if (leadControllerResourceExternalView == null) {
+LOGGER.warn("External view of {} is null.",
Helix.LEAD_CONTROLLER_RESOURCE_NAME);
Review comment:
To me the warnings for line 128, 133, 137 should be logged as error
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325438318 ## File path: pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java ## @@ -41,20 +43,25 @@ private static ControllerLeaderLocator _instance = null; public static final Logger LOGGER = LoggerFactory.getLogger(ControllerLeaderLocator.class); + // Minimum millis which must elapse between consecutive invalidation of cache + private static final long MILLIS_BETWEEN_INVALIDATE = 30_000L; + private final HelixManager _helixManager; - // Co-ordinates of the last known controller leader. - private Pair _controllerLeaderHostPort = null; + // Co-ordinates of the last known controller leader for each of the lead-controller every partitions, + // with partition number being the key and controller hostname and port pair being the value. If the lead + // controller resource is disabled in the configuration then this map contains helix cluster leader co-ordinates + // for all partitions of leadControllerResource. + private final Map> _cachedControllerLeaderMap; - // Indicates whether cached controller leader value is invalid - private volatile boolean _cachedControllerLeaderInvalid = true; + // Indicates whether cached controller leader(s) value is(are) invalid. Review comment: lol let's revert this comment change, and reversed boolean logic? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4553: Refactor ControllerLeaderLocator
Jackie-Jiang commented on a change in pull request #4553: Refactor
ControllerLeaderLocator
URL: https://github.com/apache/incubator-pinot/pull/4553#discussion_r325440281
##
File path:
pinot-core/src/main/java/org/apache/pinot/server/realtime/ControllerLeaderLocator.java
##
@@ -80,75 +87,127 @@ public static ControllerLeaderLocator getInstance() {
/**
* Locates the controller leader so that we can send LLC segment completion
requests to it.
- * Checks the {@link
ControllerLeaderLocator::_cachedControllerLeaderInvalid} flag and fetches the
leader from helix if cached value is invalid
+ * Checks the {@link ControllerLeaderLocator::_cachedControllerLeaderValid}
flag and fetches the leaders to {@link
ControllerLeaderLocator::_cachedControllerLeaderMap} from helix if cached value
is invalid
* @param rawTableName table name without type.
* @return The host-port pair of the current controller leader.
*/
public synchronized Pair getControllerLeader(String
rawTableName) {
-if (!_cachedControllerLeaderInvalid) {
- return _controllerLeaderHostPort;
+int partitionId = LeadControllerUtils.getPartitionIdForTable(rawTableName);
+if (_cachedControllerLeaderValid) {
+ return _cachedControllerLeaderMap.get(partitionId);
}
-Pair leaderForTable = getLeaderForTable(rawTableName);
-if (leaderForTable == null) {
- LOGGER.warn("Failed to find a leader for Table: {}", rawTableName);
- _cachedControllerLeaderInvalid = true;
- return null;
-} else {
- _controllerLeaderHostPort = leaderForTable;
- _cachedControllerLeaderInvalid = false;
- LOGGER.info("Setting controller leader to be {}:{}",
_controllerLeaderHostPort.getFirst(),
- _controllerLeaderHostPort.getSecond());
- return _controllerLeaderHostPort;
-}
+// No controller leader cached, fetches a fresh copy of external view and
then gets the leader for the given table.
+refreshControllerLeaderMap();
+return _cachedControllerLeaderValid ?
_cachedControllerLeaderMap.get(partitionId) : null;
}
/**
- * Firstly checks whether lead controller resource has been enabled or not.
- * If yes, use this as the leader for realtime segment completion once
partition leader exists.
- * Otherwise, try to use Helix leader.
- * @param rawTableName table name without type.
- * @return the controller leader id with hostname and port for this table,
e.g. localhost_9000
+ * Checks whether lead controller resource has been enabled or not.
+ * If yes, updates lead controller pairs from the external view of lead
controller resource.
+ * Otherwise, updates lead controller pairs from Helix cluster leader.
*/
- private Pair getLeaderForTable(String rawTableName) {
+ private void refreshControllerLeaderMap() {
// Checks whether lead controller resource has been enabled or not.
if (isLeadControllerResourceEnabled()) {
- // Gets leader from lead controller resource.
- return getLeaderFromLeadControllerResource(rawTableName);
+ refreshControllerLeaderMapFromLeadControllerResource();
} else {
- // Gets Helix leader to be the leader to this table, otherwise returns
null.
- return getHelixClusterLeader();
+ refreshControllerLeaderMapFromHelixClusterLeader();
}
}
/**
- * Checks whether lead controller resource is enabled or not. The switch is
in resource config.
+ * Updates lead controller pairs from the external view of lead controller
resource.
*/
- private boolean isLeadControllerResourceEnabled() {
-BaseDataAccessor dataAccessor =
_helixManager.getHelixDataAccessor().getBaseDataAccessor();
-Stat stat = new Stat();
+ private void refreshControllerLeaderMapFromLeadControllerResource() {
+boolean refreshSucceeded = false;
Review comment:
We don't need this extra refreshSucceeded. Just set
_cachedControllerLeaderValid to true as the last statement
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
