Skip to content
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.

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

Add test for tserver.wal.max.referenced #5250

Merged
merged 8 commits into from
Feb 4, 2025

Conversation

DomGarguilo
Copy link
Member

Fixes #5200

Adds a a test to make sure that tserver.wal.max.referenced works.

@DomGarguilo DomGarguilo added this to the 2.1.4 milestone Jan 13, 2025
@DomGarguilo DomGarguilo self-assigned this Jan 13, 2025
Comment on lines 136 to 140
int total = 0;
for (Entry<TServerInstance,List<UUID>> entry : wals.getAllMarkers().entrySet()) {
total += entry.getValue().size();
}
return total;
Copy link
Contributor

Choose a reason for hiding this comment

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

Write ahead logs can have different states. For this test do not care about the one in the open state (every tserver will always have 2 open), but want to count the non open ones.

Suggested change
int total = 0;
for (Entry<TServerInstance,List<UUID>> entry : wals.getAllMarkers().entrySet()) {
total += entry.getValue().size();
}
return total;
return wals.getAllState().values().stream().filter(walState -> walState != WalStateManager.WalState.OPEN).count();

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in a176b85


private void writeData(AccumuloClient client, String table, int startRow, int endRow)
throws Exception {
try (BatchWriter bw = client.createBatchWriter(table, new BatchWriterConfig())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could modify this method to write 2XTSERV_WAL_MAX_SIZE data to a single tablet by generating rows that fall within a target tablet and using the Mutation.estimatedMemoryUsed() method.

while(totalWritten < 2*hdfsMinBlockSize){
    Mutation m = ...
    bw.addMutation(m);
    totalWritten+=m.estimateMemoryUsed();
}

Could use the same row, it does not matter for the walog it will keep adding it even if it is the same row.

Copy link
Member Author

@DomGarguilo DomGarguilo Jan 14, 2025

Choose a reason for hiding this comment

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

Added in 3bc2502


log.info("Created table {} with splits. Now writing data.", tableName);

// Write data multiple times until we see the WAL count exceed WAL_MAX_REFERENCED
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this writes data it seems out of sync w/ the tablets. Would be good to push a lot of data to single tablet until a new non-open log has been produced and then move to the next tablet.

cfg.setProperty(Property.TSERV_WAL_MAX_SIZE, hdfsMinBlockSize);
// Set the max number of WALs that can be referenced
cfg.setProperty(Property.TSERV_WAL_MAX_REFERENCED, Integer.toString(WAL_MAX_REFERENCED));
cfg.setNumTservers(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to increase the amount of memory that tservers can use to hold new data in memory. If this is too low then tservers will minor compaction because memory is low and not because of walogs.

cfg.setProperty(Property.TSERV_MAXMEM,"256M");

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 449e457

@DomGarguilo DomGarguilo changed the base branch from main to 2.1 January 15, 2025 20:30
Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

I made these comments a long time ago, but forgot to finish the review. So they have been sitting on this PR in pending state. I just happened to notice them.

Comment on lines 81 to 83
for (int i = 1; i <= 4; i++) {
splits.add(new Text(Integer.toString(i)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding more splits would be good, could then keep writing to a different tablet.

Suggested change
for (int i = 1; i <= 4; i++) {
splits.add(new Text(Integer.toString(i)));
}
for (int i = 1; i <= 100; i++) {
splits.add(new Text(String.format("%03d", i));
}

Wait.waitFor(() -> {

// Write data that should fill or partially fill the WAL
writeData(client, tableName);
Copy link
Contributor

Choose a reason for hiding this comment

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

In earlier comment I meant we could use the same row per tablet, but did not state that. If we add the 100 tablet then this code can keep advancing tablets it will write too. The writeData can just write everything to the same row passed in.

Suggested change
writeData(client, tableName);
var rowToWrite = String.format("%03d", iteration.get());
writeData(client, tableName, rowToWrite);

@DomGarguilo
Copy link
Member Author

I made these comments a long time ago, but forgot to finish the review. So they have been sitting on this PR in pending state. I just happened to notice them.

Thanks for the suggestions. They should be incorporated as of 7170142.

@DomGarguilo DomGarguilo merged commit 4761d5a into apache:2.1 Feb 4, 2025
8 checks passed
@DomGarguilo DomGarguilo deleted the testMaxWalReferenced branch February 4, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tserver.wal.max.referenced needs a test
2 participants