Skip to content

feat: create a new metric for recording timestamp overflow cases from Hbase->Bigtable #3522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sushanb wants to merge 1 commit into from
Closed

feat: create a new metric for recording timestamp overflow cases from Hbase->Bigtable #3522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions ...ava/com/google/cloud/bigtable/hbase/replication/adapters/IncompatibleMutationAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.google.cloud.bigtable.hbase.replication.metrics.HBaseToCloudBigtableReplicationMetrics.DROPPED_INCOMPATIBLE_MUTATION_METRIC_KEY;
import static com.google.cloud.bigtable.hbase.replication.metrics.HBaseToCloudBigtableReplicationMetrics.INCOMPATIBLE_MUTATION_METRIC_KEY;
import static com.google.cloud.bigtable.hbase.replication.metrics.HBaseToCloudBigtableReplicationMetrics.INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY;

import com.google.cloud.bigtable.hbase.adapters.DeleteAdapter;
import com.google.cloud.bigtable.hbase.replication.metrics.MetricsExporter;
Expand Down Expand Up @@ -47,6 +48,11 @@ public abstract class IncompatibleMutationAdapter {
private final Connection connection;
private final Configuration conf;
private final MetricsExporter metricsExporter;
// Maximum timestamp (in usecs) that bigtable can handle while preserving lossless conversion to
// hbase timestamps in ms.
static final long BIGTABLE_MAX_TIMESTAMP = Long.MAX_VALUE - (Long.MAX_VALUE % 1000L);
Copy link
Member

@vermas2012 vermas2012 Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this logic? max timestamp in usec that BT can support is LONG.MAX_VALUE?

Copy link
Contributor Author

@sushanb sushanb Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking from hbase perspective. HBASE_EFFECTIVE_MAX_TIMESTAMP is the maximum timestamp which bigtable can accept without overflow. If the timestamp > HBASE_EFFECTIVE_MAX_TIMESTAMP, there will be overflow.

Copy link
Member

@vermas2012 vermas2012 Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that MAX timestamp bigtable can handle is Long.MAX_VALUE / 1000 I don' t understand where % comes into picture.

Copy link
Contributor Author

@sushanb sushanb Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with it. this was just pruning the last 3 digits and doing the division.

// Maximum timestamp that hbase can send to bigtable in ms.
static final long HBASE_EFFECTIVE_MAX_TIMESTAMP = BIGTABLE_MAX_TIMESTAMP / 1000L;

private void incrementDroppedIncompatibleMutations() {
metricsExporter.incCounters(DROPPED_INCOMPATIBLE_MUTATION_METRIC_KEY, 1);
Expand All @@ -56,18 +62,24 @@ private void incrementIncompatibleMutations() {
metricsExporter.incCounters(INCOMPATIBLE_MUTATION_METRIC_KEY, 1);
}


private void incrementTimestampOverflowMutations() {
metricsExporter.incCounters(INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY, 1);
}

/**
* Creates an IncompatibleMutationAdapter with HBase configuration, MetricSource, and CBT
* connection.
*
* <p>All subclasses must expose this constructor.
*
* @param conf HBase configuration. All the configurations required by subclases should come from
* here.
* @param conf HBase configuration. All the configurations required by subclases should
Copy link
Member

@vermas2012 vermas2012 Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pl fix formatting.

* come from here.
* @param metricsExporter Interface to expose Hadoop metric source present in HBase Replication
* Endpoint.
* @param connection Connection to destination CBT cluster. This reference help the subclasses to
* query destination table for certain incompatible mutation.
* Endpoint.
* @param connection Connection to destination CBT cluster. This reference help the
* subclasses to query destination table for certain incompatible
* mutation.
*/
public IncompatibleMutationAdapter(
Configuration conf, MetricsExporter metricsExporter, Connection connection) {
Expand Down Expand Up @@ -99,6 +111,11 @@ public final List<Cell> adaptIncompatibleMutations(BigtableWALEntry walEntry) {
List<Cell> returnedCells = new ArrayList<>(cellsToAdapt.size());
for (int index = 0; index < cellsToAdapt.size(); index++) {
Cell cell = cellsToAdapt.get(index);
// check whether there is timestamp overflow from HBase -> CBT
if (cell.getTimestamp() >= HBASE_EFFECTIVE_MAX_TIMESTAMP) {
incrementTimestampOverflowMutations();
}

// All puts are valid.
if (cell.getTypeByte() == KeyValue.Type.Put.getCode()) {
returnedCells.add(cell);
Expand Down Expand Up @@ -140,8 +157,9 @@ public final List<Cell> adaptIncompatibleMutations(BigtableWALEntry walEntry) {
* UnsupportedOperationException} if it can't adapt the mutation.
*
* @param walEntry the WAL entry for the cell to Adapt. The wal entry provides context around the
* cell to be adapted, things like commit timestamp and other deletes in the entry.
* @param index The index of the cell to adapt.
* cell to be adapted, things like commit timestamp and other deletes in the
* entry.
* @param index The index of the cell to adapt.
*/
protected abstract List<Cell> adaptIncompatibleMutation(BigtableWALEntry walEntry, int index);
}
3 changes: 3 additions & 0 deletions ...ogle/cloud/bigtable/hbase/replication/metrics/HBaseToCloudBigtableReplicationMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,7 @@ private HBaseToCloudBigtableReplicationMetrics() {}
public static final String INCOMPATIBLE_MUTATION_METRIC_KEY = "bigtableIncompatibleMutations";
public static final String DROPPED_INCOMPATIBLE_MUTATION_METRIC_KEY =
"bigtableDroppedIncompatibleMutations";

public static final String INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY = "bigtableIncompatibleMutationsTimestampOverflow";

}
20 changes: 20 additions & 0 deletions ...com/google/cloud/bigtable/hbase/replication/adapters/IncompatibleMutationAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.google.cloud.bigtable.hbase.replication.metrics.HBaseToCloudBigtableReplicationMetrics.DROPPED_INCOMPATIBLE_MUTATION_METRIC_KEY;
import static com.google.cloud.bigtable.hbase.replication.metrics.HBaseToCloudBigtableReplicationMetrics.INCOMPATIBLE_MUTATION_METRIC_KEY;
import static com.google.cloud.bigtable.hbase.replication.metrics.HBaseToCloudBigtableReplicationMetrics.INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -226,4 +227,23 @@ public void testIncompatibleDeletesAreDropped() {
verify(metricsExporter, times(1)).incCounters(INCOMPATIBLE_MUTATION_METRIC_KEY, 1);
verify(metricsExporter, times(1)).incCounters(DROPPED_INCOMPATIBLE_MUTATION_METRIC_KEY, 1);
}

@Test
public void testTimestampsOverflowMutations() {
ArrayList<Cell> walEntryCells = new ArrayList<>();
Cell put1 = new KeyValue(rowKey, cf, qual, Long.MAX_VALUE, KeyValue.Type.Put, val);
Cell put2 = new KeyValue(rowKey, cf, qual, 0, KeyValue.Type.Put, val);

walEntryCells.add(put1);
walEntryCells.add(put2);
BigtableWALEntry walEntry =
new BigtableWALEntry(System.currentTimeMillis(), walEntryCells, tableName);

Assert.assertEquals(
Arrays.asList(put1, put2), incompatibleMutationAdapter.adaptIncompatibleMutations(walEntry));

verify(metricsExporter).incCounters(INCOMPATIBLE_MUTATION_METRIC_KEY, 0);
Copy link
Member

@vermas2012 vermas2012 Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we not increment INCOMPATIBLE_MUTATION_COUNT metric? This is an incompatible mutation, right?

Copy link
Contributor Author

@sushanb sushanb Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of creating a different metric INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY for flagging this.

Should INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY and INCOMPATIBLE_MUTATION_COUNT be exclusive?

Copy link
Member

@vermas2012 vermas2012 Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't need to be exclusive. If we go this route, it maybe helpful to then log metrics for all the incompatible mutation types.

Copy link
Member

@vermas2012 vermas2012 Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhambleton comments?

Copy link
Contributor Author

@sushanb sushanb Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be helpful for customers if we log metrics for each incompatible type.

verify(metricsExporter).incCounters(DROPPED_INCOMPATIBLE_MUTATION_METRIC_KEY, 0);
verify(metricsExporter).incCounters(INCOMPATIBLE_MUTATION_TIMESTAMP_OVERFLOW_METRIC_KEY, 1);
}
}