haridsv commented on code in PR #2379:
URL: https://github.com/apache/phoenix/pull/2379#discussion_r3021869474
##########
phoenix-core-client/src/main/java/org/apache/phoenix/util/SHA256DigestUtil.java:
##########
@@ -32,53 +28,49 @@
public class SHA256DigestUtil {
/**
- * Maximum allowed size for encoded SHA-256 digest state. SHA-256 state is
~96 bytes, we allow up
- * to 128 bytes as buffer.
+ * Maximum allowed size for encoded SHA-256 digest state. BouncyCastle's
SHA256Digest encoded
+ * state ranges from 53 to 113 bytes (52 base + 0-60 buffered words + 1
purpose byte). We allow up
+ * to 128 bytes as headroom.
*/
public static final int MAX_SHA256_DIGEST_STATE_SIZE = 128;
/**
- * Encodes a SHA256Digest state to a byte array with length prefix for
validation. Format: [4-byte
- * integer length][encoded digest state bytes]
+ * Encodes a SHA256Digest state to a byte array.
* @param digest The digest whose state should be encoded
- * @return Byte array containing integer length prefix + encoded state
+ * @return Byte array containing the raw BouncyCastle encoded state
*/
public static byte[] encodeDigestState(SHA256Digest digest) {
byte[] encoded = digest.getEncodedState();
- ByteBuffer buffer = ByteBuffer.allocate(Bytes.SIZEOF_INT + encoded.length);
- buffer.putInt(encoded.length);
- buffer.put(encoded);
- return buffer.array();
+ if (encoded.length > MAX_SHA256_DIGEST_STATE_SIZE) {
+ throw new IllegalArgumentException(
+ String.format("SHA256 encoded state too large: %d, expected <= %d",
encoded.length,
+ MAX_SHA256_DIGEST_STATE_SIZE));
+ }
Review Comment:
This check makes no sense to me, can you explain what exactly are you
protecting against? Also not sure about the exception type implies.
##########
phoenix-core-server/src/main/java/org/apache/phoenix/mapreduce/PhoenixSyncTableCheckpointOutputRow.java:
##########
@@ -143,27 +143,91 @@ public String getCounters() {
@VisibleForTesting
public long getSourceRowsProcessed() {
- return
parseCounterValue(PhoenixSyncTableMapper.SyncCounters.SOURCE_ROWS_PROCESSED.name());
+ return CounterFormatter.parseSourceRows(counters);
}
@VisibleForTesting
public long getTargetRowsProcessed() {
- return
parseCounterValue(PhoenixSyncTableMapper.SyncCounters.TARGET_ROWS_PROCESSED.name());
+ return CounterFormatter.parseTargetRows(counters);
}
- private long parseCounterValue(String counterName) {
- if (counters == null || counters.isEmpty()) {
- return 0;
+ /**
+ * Utility class for formatting and parsing counter strings. Encapsulates
the counter format
+ * contract to ensure consistency between formatting (in mapper) and parsing
(in tests).
+ */
+ public static class CounterFormatter {
+ private static final String FORMAT_CHUNK = "%s=%d,%s=%d";
+ private static final String FORMAT_MAPPER = "%s=%d,%s=%d,%s=%d,%s=%d";
+
+ /**
+ * Formats chunk counters as comma-separated key=value pairs.
+ * @param sourceRows Source rows processed
+ * @param targetRows Target rows processed
+ * @return Formatted string:
"SOURCE_ROWS_PROCESSED=123,TARGET_ROWS_PROCESSED=456"
+ */
+ public static String formatChunk(long sourceRows, long targetRows) {
+ return String.format(FORMAT_CHUNK,
+ PhoenixSyncTableMapper.SyncCounters.SOURCE_ROWS_PROCESSED.name(),
sourceRows,
+ PhoenixSyncTableMapper.SyncCounters.TARGET_ROWS_PROCESSED.name(),
targetRows);
+ }
+
+ /**
+ * Formats mapper counters as comma-separated key=value pairs.
+ * @param chunksVerified Chunks verified count
+ * @param chunksMismatched Chunks mismatched count
+ * @param sourceRows Source rows processed
+ * @param targetRows Target rows processed
+ * @return Formatted string with all mapper counters
+ */
+ public static String formatMapper(long chunksVerified, long
chunksMismatched, long sourceRows,
+ long targetRows) {
+ return String.format(FORMAT_MAPPER,
+ PhoenixSyncTableMapper.SyncCounters.CHUNKS_VERIFIED.name(),
chunksVerified,
+ PhoenixSyncTableMapper.SyncCounters.CHUNKS_MISMATCHED.name(),
chunksMismatched,
+ PhoenixSyncTableMapper.SyncCounters.SOURCE_ROWS_PROCESSED.name(),
sourceRows,
+ PhoenixSyncTableMapper.SyncCounters.TARGET_ROWS_PROCESSED.name(),
targetRows);
+ }
+
+ /**
+ * Parses SOURCE_ROWS_PROCESSED value from counter string.
+ * @param counters Counter string in format "KEY1=val1,KEY2=val2,..."
+ * @return Source rows processed, or 0 if not found
+ */
+ public static long parseSourceRows(String counters) {
+ return parseCounterValue(counters,
+ PhoenixSyncTableMapper.SyncCounters.SOURCE_ROWS_PROCESSED.name());
+ }
+
+ /**
+ * Parses TARGET_ROWS_PROCESSED value from counter string.
+ * @param counters Counter string in format "KEY1=val1,KEY2=val2,..."
+ * @return Target rows processed, or 0 if not found
+ */
+ public static long parseTargetRows(String counters) {
+ return parseCounterValue(counters,
+ PhoenixSyncTableMapper.SyncCounters.TARGET_ROWS_PROCESSED.name());
}
- String[] pairs = counters.split(",");
- for (String pair : pairs) {
- String[] keyValue = pair.split("=");
- if (keyValue.length == 2 && keyValue[0].trim().equals(counterName)) {
- return Long.parseLong(keyValue[1].trim());
+ /**
+ * Parses a specific counter value from the comma-separated counter string.
+ * @param counters Counter string in format "KEY1=val1,KEY2=val2,..."
+ * @param counterName Name of the counter to extract
+ * @return Counter value, or 0 if not found
+ */
+ private static long parseCounterValue(String counters, String counterName)
{
+ if (counters == null || counters.isEmpty()) {
+ return 0;
}
+
+ String[] pairs = counters.split(",");
+ for (String pair : pairs) {
+ String[] keyValue = pair.split("=");
+ if (keyValue.length == 2 && keyValue[0].trim().equals(counterName)) {
+ return Long.parseLong(keyValue[1].trim());
+ }
Review Comment:
Else, you should treat it as a corruption and throw an exception.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java:
##########
@@ -1207,8 +1207,9 @@ public static boolean isIndexRebuild(Scan scan) {
return
scan.getAttribute((BaseScannerRegionObserverConstants.REBUILD_INDEXES)) != null;
}
- public static boolean isSyncTableChunkFormation(Scan scan) {
- return
scan.getAttribute(BaseScannerRegionObserverConstants.SYNC_TABLE_CHUNK_FORMATION)
!= null;
+ public static boolean isSyncTableChunkFormationEnabled(Scan scan) {
+ return Arrays.equals(
+
scan.getAttribute(BaseScannerRegionObserverConstants.SYNC_TABLE_CHUNK_FORMATION),
TRUE_BYTES);
Review Comment:
I would suggest to also rename the attribute to indicate that it is a
boolean.
--
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]
