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.

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

Jump to bottom

cloud: Address some warnings #16988

Merged
merged 3 commits into from
Mar 25, 2024
Merged

cloud: Address some warnings #16988

merged 3 commits into from
Mar 25, 2024

Conversation

BenPope
Copy link
Member

@BenPope BenPope commented Mar 10, 2024

Spotted with Clang 18

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@andijcr
Copy link
Contributor

andijcr commented Mar 10, 2024

for cstore: most likely it's fine we need to check if it supports the copyable semantics

dotnwat
dotnwat reviewed Mar 11, 2024
View reviewed changes
src/v/archival/ntp_archiver_service.cc Outdated Show resolved Hide resolved
src/v/cloud_storage/segment_meta_cstore.h Outdated Show resolved Hide resolved
@dotnwat dotnwat requested a review from andijcr March 11, 2024 15:51
@andijcr andijcr requested a review from dotnwat March 11, 2024 15:57
@rockwotj
Copy link
Contributor

Since @BenPope is at a conference this week, I am going to push this through (hope that's OK Ben!) as I am now running on clang 18 😄

@BenPope
Copy link
Member Author

BenPope commented Mar 19, 2024

Since @BenPope is at a conference this week, I am going to push this through (hope that's OK Ben!) as I am now running on clang 18 😄

I can fixup the commented lock vector and push it in about 80mins.

@BenPope
Copy link
Member Author

BenPope commented Mar 19, 2024

Since @BenPope is at a conference this week, I am going to push this through (hope that's OK Ben!) as I am now running on clang 18 😄

I can fixup the commented lock vector and push it in about 80mins.

Oh I see you force pushed it. Doesn't it just complain that _ is unused?

@BenPope
Copy link
Member Author

BenPope commented Mar 19, 2024

I am going to push this through (hope that's OK Ben!) as I am now running on clang 18

I think we still want storage to tick the copyable iterator?

@rockwotj
Copy link
Contributor

Yeah I changed it to std ignore

@BenPope
Copy link
Member Author

BenPope commented Mar 19, 2024

Yeah I changed it to std ignore

Works for me, but then so did my suggestion of auto hold_locks{std::move(segment_read_locks});, I really don't have a strong opinion here.

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 20, 2024 •
edited

new failures in https://buildkite.com/redpanda/redpanda/builds/46467#018e58f5-19ec-4814-8d68-2793fe0dec21:

"rptest.tests.offset_for_leader_epoch_test.OffsetForLeaderEpochTest.test_offset_for_leader_epoch"

new failures in https://buildkite.com/redpanda/redpanda/builds/46585#018e62a6-9fc6-4078-87fc-ad784863e3b1:

"rptest.tests.read_replica_e2e_test.TestReadReplicaService.test_identical_lwms_after_delete_records.partition_count=5.cloud_storage_type=CloudStorageType.S3"

@BenPope @rockwotj
cloud: Address [-Werror,-Wunused-parameter]
3b1bcdb 
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope @rockwotj
cloud: Address [-Wc++11-narrowing-const-reference]
7950cc4 
Signed-off-by: Ben Pope <ben@redpanda.com>
@rockwotj rockwotj force-pushed the clang_18_cloud branch 2 times, most recently from 4c9413c to 6880fca Compare March 21, 2024 15:59
@rockwotj
Copy link
Contributor

Force push: review feedback

@rockwotj rockwotj requested a review from dotnwat March 21, 2024 16:00
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 21, 2024 •
edited

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46570#018e6200-d022-46fc-acf3-c7a1b04c5cef

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46585#018e6293-c1e9-47f3-a9e0-9178ff767133

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46585#018e7672-9c10-46e8-8117-6dbb63dea2d5

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46585#018e7672-9c0d-4348-acd8-5fde96783d5f

dotnwat
dotnwat reviewed Mar 21, 2024
View reviewed changes
src/v/test_utils/archival.h Show resolved Hide resolved
src/v/cloud_storage/segment_meta_cstore.cc Outdated Show resolved Hide resolved
@rockwotj
cloud: explcitly implment std::equal
03d5cd8 
libc++18 requires that these iterators are copyable, so manually
implement that to workaround the need to make the iterators copyable.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj
Copy link
Contributor

Force push: Fix dunder

@rockwotj rockwotj requested a review from dotnwat March 21, 2024 18:59
@rockwotj
Copy link
Contributor

CI Failures: #14139, #17247

Unit test failure is for transforms, which I am tracking here and will fix in a followup: #17367

@rockwotj rockwotj merged commit e64151c into redpanda-data:dev Mar 25, 2024
17 checks passed
@BenPope BenPope self-assigned this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rockwotj rockwotj rockwotj left review comments

@mmaslankaprv mmaslankaprv mmaslankaprv left review comments

@dotnwat dotnwat dotnwat approved these changes

@andijcr andijcr Awaiting requested review from andijcr

Assignees

@BenPope BenPope

Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@BenPope @andijcr @rockwotj @vbotbuildovich @dotnwat @mmaslankaprv