Skip to content

Add kiro steering docs #6280

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add kiro steering docs #6280

wants to merge 1 commit into from

Conversation

zoewangg
Copy link
Contributor

Motivation and Context

Add kiro steering docs. I added existing guidelines from docs folder and internal docs. I also added additional guidelines that came to my mind.

@zoewangg zoewangg requested a review from a team as a code owner July 18, 2025 22:57
Copy link

return fallbackData();
}, executor);
```
- **Use appropriate completion methods**:
Copy link
Contributor

Choose a reason for hiding this comment

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

what about whenComplete? We use it a lot

- Include comprehensive Javadoc for public APIs
- Keep methods short and focused on a single responsibility
- Limit the number of method parameters (ideally 3 or fewer)
- Use appropriate access modifiers (private, protected, public)
Copy link
Contributor

Choose a reason for hiding this comment

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

will Kiro know what is "appropriate" or should we elaborate?

- If the class does not implement `Supplier`: `{Noun}Provider` (e.g. `AwsCredentialsProvider`)
- If the "get" method has parameters: `{Noun}Factory` (e.g. `AwsJsonProtocolFactory`)

#### Service-specific classes
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this section seems a bit restrictive? We don't want any other options?

```
- `equals()`: Compare all fields for equality, including proper null handling
- `hashCode()`: Include all fields in the hash code calculation
- Consider using IDE-generated implementations
Copy link
Contributor

Choose a reason for hiding this comment

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

SDK ToString utils for toString()?

- `Optional` **SHOULD** be used when it is not obvious to a caller whether a result will be null
- `Optional` **MUST NOT** be used for "getters" in generated service model classes
- For member variables: `Optional` **SHOULD NOT** be used
- For method parameters: `Optional` **MUST NOT** be used
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we provide guidance to use @nullable instead or just not doing it?

- All public POJO classes **MUST** implement `toString()`, `equals()`, and `hashCode()` methods
- When adding new fields to existing POJO classes, these methods **MUST** be updated to include the new fields
- Implementation guidelines:
- `toString()`: Include class name and all fields with their values
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible additional guideline - do not include fields that could be considered sensitive such as credentials

```
- `equals()`: Compare all fields for equality, including proper null handling
- `hashCode()`: Include all fields in the hash code calculation
- Consider using IDE-generated implementations
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this guidance useful for kiro? I think we often use the intelliJ auto-generated implementations, but not sure that will apply well here. I think the guidance maybe should be either: use java.util.Objects helpers or write explicit code.

@RanVaknin
Copy link
Contributor

RanVaknin commented Jul 22, 2025

Should we add a mixed version compatibility guidance?

## Version Compatibility

- **Default Behavior Substitution**: Never replace a conditional code path with an unconditional one without thorough compatibility testing. When a feature flag is removed and a previously optional code path becomes the only path, this constitutes a high-risk change that can break compatibility with older service modules.

- **Method Invocation Pattern Changes**: Be extremely cautious when changing which methods core components call on service interfaces. Even if the interface method exists in older versions, the implementation might throw UnsupportedOperationException or behave differently than expected by newer core components.

- **Interface Default Methods**: When adding methods to shared interfaces that will be called by core components:
  - Implement graceful fallback behavior in core when methods might not be properly implemented when possible.
  - Test with older service versions to verify runtime behavior, not just compile-time compatibility

- **Behavioral Assumptions**: Explicitly document and test assumptions about how service modules implement interface methods. Core components should not make new assumptions about service module behavior without ensuring backward compatibility.

- **Risk Assessment for Code Cleanup**: What appears to be simple "cleanup" or "optimization" can have significant compatibility implications. Always evaluate the runtime behavior changes, not just the code structure changes.

@@ -0,0 +1,133 @@
---
title: Asynchronous Programming Guidelines for AWS SDK v2
Copy link
Contributor

@dagnir dagnir Jul 24, 2025

Choose a reason for hiding this comment

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

Doesn't have to be in this PR but I think some guidance around locking and shared resources would be good too. For example w.r.t issues we had around making sure SdkUri/BoundedCache are performant

future.thenAccept(result -> processResult(result));
```
- **Add stacktrace to exceptions**: When using `CompletableFuture#join`, use `CompletableFutureUtils#joinLikeSync` to preserve stacktraces
- **Don't ignore results**: Never ignore the result of a new `CompletionStage` if the parent stage can fail (unless you're absolutely sure it's safe to)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to add an example for this one, it might not be immediately apparent. e.g.

        CountDownLatch latch = new CountDownLatch(1);

        CompletableFuture<URL> future = new CompletableFuture<>();
        
        future.thenRun(url -> {
            downloadUrl(url);
            latch.countDown();
        });
        
        latch.await();

- Avoid excessive stubbing - it can make tests brittle and hard to maintain
- [Wiremock](https://wiremock.org/) is commonly used to mock server responses in functional tests

## Testing Asynchronous Code
Copy link
Contributor

Choose a reason for hiding this comment

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

Another useful method is sometimes we run hundreds/thousands of iterations of a test to ensure that none of the iterations fail, which is helpful for testing race conditions

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.

5 participants